Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix spectral extraction previews on unit change #3157

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Aug 19, 2024

Description

This pull request updates the spectral extraction live-preview for any change that affects the y-units of the extracted spectrum or spectral viewer (and also ensures the internal units in the plugin are populated for the warning banner introduced in #3143 which was recently broken).

Screen.Recording.2024-08-19.at.2.28.41.PM.mov

Change log entry

  • Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
    list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
    should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@kecnry kecnry added this to the 4.0 milestone Aug 19, 2024
@github-actions github-actions bot added cubeviz specviz plugin Label for plugins common to multiple configurations labels Aug 19, 2024
Comment on lines -112 to +114
flux_unit = Unicode().tag(sync=True)
sb_unit = Unicode().tag(sync=True)
flux_units = Unicode().tag(sync=True)
sb_units = Unicode().tag(sync=True)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to be consistent with the traitlets above, I was confusing myself when writing code later in the plugin and having to switch between singular and plural

@@ -542,7 +545,9 @@ def _preview_x_from_extracted(self, extracted):
return extracted.spectral_axis.value

def _preview_y_from_extracted(self, extracted):
return extracted.flux.value
# TODO: use extracted's PIXAR_SR instead (but for some reason isn't populated here...)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does anyone know why this is (I'm guessing its just populated later when actually loading into the app)? Ultimately, I think the extracted spectrum just gets a copy of PIXAR_SR from the input cube, so this shouldn't matter, it just would read more cleanly if it did.

@kecnry kecnry force-pushed the spec-ext-previews branch 2 times, most recently from 56ed577 to 67df70a Compare August 19, 2024 18:35
@kecnry kecnry marked this pull request as ready for review August 19, 2024 18:41
@kecnry
Copy link
Member Author

kecnry commented Aug 19, 2024

moving back to draft - the test failures are all related to non per-steradian SB units, which will be addressed by #3156. Instead of writing a temporary workaround, feel free to test/review the diff and we can rebase this once #3156 is in.

@kecnry kecnry marked this pull request as draft August 19, 2024 19:04
@rosteen
Copy link
Collaborator

rosteen commented Aug 21, 2024

It seems like changing to flux makes the preview disappear (as expected, I think), but then changing back to surface brightness doesn't trigger the preview to reappear. It only comes back if you then change the flux unit while in surface brightness mode.

@kecnry
Copy link
Member Author

kecnry commented Aug 21, 2024

is it possible that its timing out? Do you get the warning that the previews have been disabled?

@rosteen
Copy link
Collaborator

rosteen commented Aug 21, 2024

The previews disabled warning does not appear, I do see the warning about the display units (surface brightness vs flux). And now that I think about it, I guess the preview should show up when flux is selected in unit conversion. It doesn't seem to be timing out.

I was able to get the preview to show this time. You can see that at first it flickered and then disappeared when I switched to flux, and when I changed units (the second time?) it stuck around:

Screen.Recording.2024-08-21.at.4.52.46.PM.mov

Copy link
Contributor

@cshanahan1 cshanahan1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you load in a surface brightness cube, the automatic sum extraction is plotted in the spectral viewer and the spectral y axis correctly says flux, however in the unit conversion app the toggle is set to 'surface brightness', i don't see this on main

@kecnry
Copy link
Member Author

kecnry commented Oct 2, 2024

@cshanahan1 - I think this is fixed now after a rebase, but please confirm if you have a second to test.

@kecnry
Copy link
Member Author

kecnry commented Oct 2, 2024

(still working on some cases where the preview is not showing after the rebase)

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.47%. Comparing base (fc88465) to head (9e56b9f).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3157      +/-   ##
==========================================
+ Coverage   88.46%   88.47%   +0.01%     
==========================================
  Files         125      125              
  Lines       18677    18700      +23     
==========================================
+ Hits        16522    16545      +23     
  Misses       2155     2155              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kecnry kecnry marked this pull request as ready for review October 3, 2024 13:35
Copy link
Contributor

@cshanahan1 cshanahan1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to work as intended - I can't convert to erg/s/cm2/a but i think that will be fixed by mine and @pllim PRs

Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me now, thanks.

@kecnry kecnry merged commit 770dfd1 into spacetelescope:main Oct 7, 2024
25 checks passed
@kecnry kecnry deleted the spec-ext-previews branch October 7, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cubeviz no-changelog-entry-needed changelog bot directive plugin Label for plugins common to multiple configurations Ready for final review specviz
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants