-
Notifications
You must be signed in to change notification settings - Fork 75
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
add spectral unit to moment map 0 display unit, simplify logic #3211
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3211 +/- ##
==========================================
- Coverage 88.51% 88.50% -0.01%
==========================================
Files 125 125
Lines 18694 18697 +3
==========================================
+ Hits 16547 16548 +1
- Misses 2147 2149 +2 ☔ View full report in Codecov by Sentry. |
full_spectrum = self.app._jdaviz_helper.get_data(self.dataset.selected, | ||
use_display_units=True) | ||
full_spectrum = self.app._jdaviz_helper.get_data(self.dataset.selected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this (and the other removal below) because the marks are handling the unit conversion themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i believe so (but also cc @bmorris3) . I also verified this change doesn't break anything in line analysis where this mixin is also used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also see https://jira.stsci.edu/browse/JDAT-4830, this bug was not introduced with this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #3212 for the fix to moment map continuum previews during unit conversion. Whichever PR makes it in second should rebase and ensure that is still working as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - would like to have @bmorris3 review before merging, if possible, just to make sure we're not missing anything from that original PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some differences with #3116 for @cshanahan1 to double check. Otherwise all good by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes not carried over from #3116 for review:
- https://github.com/spacetelescope/jdaviz/pull/3116/files#diff-44f616436de8315fb25244c8555ea83596e03bedf23685221f4becc69bad8fbbR32
- https://github.com/spacetelescope/jdaviz/pull/3116/files#diff-44f616436de8315fb25244c8555ea83596e03bedf23685221f4becc69bad8fbbR112
- (not sure if this one is still needed?) https://github.com/spacetelescope/jdaviz/pull/3116/files#diff-44f616436de8315fb25244c8555ea83596e03bedf23685221f4becc69bad8fbbR121-R122
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- fixed, removed Flux from options
- i see this correctly removing flux as an option already
- I intentionally omitted this because I found it to work without it, and I new @kecnry did some work recently on units with an empty viewer so I figured thats why it was now working
I have no idea why 3.12 tests are failing |
I think scipy dev wheel is broken again. You can ignore it. |
in that case im going to merge this, if i can find some consequence of not subscribing to AddDataMessage i'll follow up but i don't see any issues |
Co-authored-by: bmorris3 <bmmorris@stsci.edu>
There was previously an issue where the moment map plugin was using the spectral y axis unit for moment 0 rather than always using a surface brightness unit. This was fixed in #3156.
This PR applies some of the changes from #3116, which also fixed that issue but based on an outdated main pre many unit conversion changes, so I found it easier to cherry pick some of the code here rather than rebasing that PR.
The display unit now has the spectral unit because specutils is pinned to the version with the fix for MM units. I made some small tweaks including using _get_display_unit rather than grabbing the UC plugin from the MM plugin and accessing display unit there.
I think sufficient test coverage was added in #3156 so i didn't add the tests from #3116, but i did expand one of the tests to test that the moment map plugin has the correct units when spectral unit is changed (previously was just testing when flux unit was changed)
This will close JDAT 4651 and #3116
Closes #3116.