-
Notifications
You must be signed in to change notification settings - Fork 73
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
Skip flux conversion for moment maps #3158
Skip flux conversion for moment maps #3158
Conversation
Whoops, it almost certainly does. I should have stuck to fixing the first bug I found 😆 |
I could split out the changes to |
24adaa6
to
04d321c
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.
LGTM.
If there's any way to get an easy regression test, that would be great to add!
if str(u.Unit(unit).physical_type) not in ("spectral flux density", | ||
"surface brightness"): | ||
skip_spectral_density_eqv = True |
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.
if this is the only exception, this probably could be shortened to be directly in the if-statement itself, but that's probably a matter of preference.
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. Thanks!
There is a conflict. Please rebase. |
04d321c
to
535727f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3158 +/- ##
=======================================
Coverage 88.42% 88.42%
=======================================
Files 122 122
Lines 18264 18267 +3
=======================================
+ Hits 16150 16153 +3
Misses 2114 2114 ☔ View full report in Codecov by Sentry. |
* Skip flux conversion for moment maps * More general check on unit physical type
Looks like #3139 broke adding moment maps to a viewer, due to the
coords_info
logic. This skips the flux conversion that breaks it in the case of moment-like units (velocity and higher powers of it) by skipping any unit with incompatible physical type.Additionally, calculating moment 0 in flux when the cube is in surface brightness seemed to be broken (again?) so I moved the unit conversion to be before the moment calculation, so we can convert, e.g., MJy/sr to MJy and not have to worry about the extra wavelength unit that comes in via the moment calculation.I also took this opportunity to bump the specutils pin and get rid of the handling for the change in 1.16 for the dev tests and such.