Enable contour unit conversion #3149
Conversation
rosteen
added
Upstream fix required
plugin
|
I cancelled the CI for now, since we know they'll fail without the glue-jupyter release. |
rosteen
requested review from
javerbukh,
pllim,
kecnry,
bmorris3,
haticekaratay,
cshanahan1 and
gibsongreen
as code owners
|
Marked as ready for review, reminder that you need |
|
Are we then stuck until glue release happens? 😏 p.s. Please rebase to remove change log conflict. |
rosteen
force-pushed
the
contour-unit-conversion
branch
from
83553bf to
ab25778
Compare
jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py
Outdated
Show resolved
Hide resolved
| # DQ layer doesn't play nicely with this attribute | ||
| if "DQ" in layer.layer.label: | ||
| continue |
There was a problem hiding this comment.
I'm guessing masks don't either.... could we instead check the units on the layer to make sure they're surface brightness? Eventually we might need to generalize this method to also set display units for images in velocity, etc (from moment maps), but that isn't enabled in unit conversion yet, so is probably safe to ignore.
There was a problem hiding this comment.
I added a second check below for surface brightness - we know DQ doesn't work, so might as well do the fast check first for that still?
jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
| # Always send a surface brightness unit to contours | ||
| if self.flux_or_sb_selected == 'Flux': | ||
| yunit = self._append_angle_correctly(yunit, self.angle_unit.selected) | ||
| self._find_and_convert_contour_units(yunit=yunit) |
There was a problem hiding this comment.
what if _find_and_convert_contour_units just observed sb_unit_selected which already handles this logic?
There was a problem hiding this comment.
we discussed that unit conversion probably needs some code moving soon anyways, so I'm fine deferring this until then
|
(looks like a test needs updating before merge, but assuming that is just the typo fix, approval stands) |
It's a real failure, I didn't account for subsets existing when units are converted. Should be a quick fix. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3149 +/- ##
==========================================
- Coverage 88.82% 87.21% -1.62%
==========================================
Files 112 122 +10
Lines 17626 18284 +658
==========================================
+ Hits 15657 15947 +290
- Misses 1969 2337 +368 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
* Add Jesse's first pass code * Debugging * Debugging * No longer errors but the contour labels still don't update * Now working with glue-jupyter main, added test * Changelog * Codestyle * Remove debugging print statements * Update glue-jupyter pin * Don't loop through all viewers if not necessary * Skip anything not in surface brightness * Update jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> * Skip subset layers * Codestyle --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
This pulls out the contour-relevant code from #2702 and resolves a couple issues I ran into, as well as adding a test for the contour unit conversion. Requires glue-viz/glue-jupyter#461 so we'll need a glue-jupyter release.