-
Notifications
You must be signed in to change notification settings - Fork 74
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
Untranslatable unit handling #3139
Untranslatable unit handling #3139
Conversation
dd8189c
to
14a5b9b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3139 +/- ##
==========================================
- Coverage 88.82% 88.81% -0.01%
==========================================
Files 112 112
Lines 17586 17624 +38
==========================================
+ Hits 15621 15653 +32
- Misses 1965 1971 +6 ☔ View full report in Codecov by Sentry. |
Patch coverage is only 67.64%. Any chance you can increase that? |
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.
Works in testing, I left a few comments/suggestions/questions. I also think that the code handling the untranslatable units in utils.py
could be reorganized a bit to reduce repeated code, I may make a PR to your branch as a suggestion since it's a bit too much to do in a comment here.
else: | ||
spectral_values = spec.spectral_axis | ||
|
||
spec_unit = str(spec.flux.unit) |
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.
I was going to say this should probably be flux_unit
, but I guess the point is that it could be surface brightness so calling it that would also be confusing. How about spec_y_unit
?
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.
For this spec unit, it's specifically looking at the Spectrum1D object. We want to see what the native units of the input data are, since that ultimately dictate the conversions/translations that take place. We can't just look at what's displayed along the spectrum viewers y-axis. I added a comment above the line that should clarify that the unit can be flux or sb, but let me know if I should add or explain this better
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.
we just want to avoid confusion with spectral_unit
/spectral_axis
used in many places that are referring to wavelength/frequency.
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.
Exactly. fluxish_unit
? 😆
I didn't realize when I wrote the original comment that this variable is only used once later in the method, so it's not a huge deal.
I see it at 79.62...would still be nice to increase it a bit if possible. |
I changed the term 'untranslatable' to 'transitive' for those specific units since they now are translatable. I was also thinking 'indirect', but if there are any other suggestions I can update the name |
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.
Left a couple more small comments, I think this is close but I want to think about the _transitive_conversion
function a little bit more, I think that's where some consolidation could happen but I'm not sure (and it works, so in some sense it doesn't necessarily need to be prettier).
This comment was marked as resolved.
This comment was marked as resolved.
I reproduced and believe I know what the issue is, the spectra if you use min/max/mean are in Surface Brightness units and there needs to be an extra case in _indirect_conversions to handle going from A->D (just as it is done in the image-viewers) and not just A->C. |
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.
Mostly minor comments.
Side note: If performance becomes a problem in the future, we can take shortcuts for certain cases because when a /sr
converts to another /sr
, you don't really need all the fancy indirect stuff. It is basically the same as silently removing /sr
, do the normal flux conversion, and then silently reapply it.
Hmm looks like there are merge conflicts now, so needs a rebase too.
Personally, I prefer logical grouping but I cannot speak for everyone. |
Can ordering/grouping be a follow-up (and probably not necessary for 4.0)? |
…us mouseover global messages
e14f4c1
to
ea76c26
Compare
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.
My remaining unresolved comments are minor and spectral extraction bug seems fixed, so approving. Thanks!
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.
Just a few minor comments from looking through the diff, but I think they can be addressed as a follow-up since @gibsongreen is away for a while.
slice : `astropy.units.Quantity`, optional | ||
The current slice of a data cube, with units. Necessary for complex unit | ||
conversions/translations that require spectral density equivalencies. |
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.
could this be unitless and just documented as required to be in the same units as original_units
/values
? That would then save a call to app._get_display_units
when calling this.
if self.angle_unit.selected == 'pix': | ||
mouseover_unit = self.flux_unit.selected | ||
else: | ||
mouseover_unit = self.sb_unit_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.
@cshanahan1 - this logic will probably need to change for your effort once images/cubes are guaranteed to always be in SB units
# if cube was loaded in flux units, we still need to broadcast | ||
# a 'sb' message for mouseover info. this should be removed when | ||
# unit change messaging is improved and is a temporary fix |
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.
FYI @cshanahan1
Description
This pull request is to address implementing a solution to handle units that cannot be translated using the MJy <-> MJy / sr custom equivalency (and subsequent surface brightness conversions between these units). The following units are able to be converted between flux units and other flux units, but issues specifically arise in translations and in surface brightness to surface brightness conversions. Custom equivalencies have been added in this PR that enable conversions and translations to solve this issue, and they work in a notebook cell setting. However, attempting to utilize this solution in Cubeviz UI causes tracebacks. It is likely due to the native units from the data collection. The native units will be used for the Spectrum1D object used in
flux_conversion
, and the native unit will also be used as either theoriginal_unit
or thetarget_unit
(see jdaviz/utils.py). Translation(s) and/or conversion(s) needs to take place before the next translation/conversion occurs.Change log entry
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, maintainershould 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.
trivial
label.