POC: Add concept notebook using glue unit conversion #1398
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1398 +/- ##
=======================================
Coverage 84.91% 84.91%
=======================================
Files 91 91
Lines 8288 8288
=======================================
Hits 7038 7038
Misses 1250 1250 Continue to review full report at Codecov.
|
| "class UnitConverterWithTemperature:\n", | ||
| "\n", | ||
| " def equivalent_units(self, data, cid, units):\n", | ||
| " equivalencies = u.temperature() if 'temp' in cid.label.lower() else None\n", |
There was a problem hiding this comment.
how will this work in practice (we don't want all equivalencies for all axes)? Is the lack of the spectral equivalency here the reason Hz is not converting as expected?
There was a problem hiding this comment.
Both good questions. I will look into the second one, no good answer for the first one.
There was a problem hiding this comment.
Maybe a lookup dictionary of some sort to map conv type to equivalencies, or a bunch of if-else logic blocks?
FWIW, if-else is applied at https://github.com/spacetelescope/synphot_refactor/blob/2a382d7bdf29cc4a1e6b69e59d5c1d0f82dabffc/synphot/units.py#L242
There was a problem hiding this comment.
The reason this doesn't work currently is because you should indeed be using the spectral equivalency for some components - here's a simple example that works for this particular example:
from astropy import units as u
from glue.core.units import unit_converter
@unit_converter('custom-astropy')
class UnitConverterWithSpectral:
def equivalent_units(self, data, cid, units):
equivalencies = u.spectral() if cid.label.lower() in ['freq', 'wave'] else None
return map(str, u.Unit(units).find_equivalent_units(include_prefix_units=True, equivalencies=equivalencies))
def to_unit(self, data, cid, values, original_units, target_units):
equivalencies = u.spectral() if cid.label.lower() in ['freq', 'wave'] else None
return (values * u.Unit(original_units)).to_value(target_units, equivalencies=equivalencies)
|
@javerbukh I've tested this with the latest version of glue-core and glue-jupyter but ran into issues because the conversion isn't right for spectral axes - see https://github.com/spacetelescope/jdaviz/pull/1398/files#r1085558149 - I also couldn't get the data to show up in specviz but I think that's just because I wasn't sure what UI steps I should do in addition to the notebook code. Maybe you could give this a spin with the updated unit converter above and the latest glue-core and glue-jupyter branches? |
|
I was able to get the unit conversion working when following one of the tests you added to glue-jupyter: As for adding data in the UI, you may need to go to the data dropdown (top left of viewer), click the arrows pointing down, and then add those data using the plus icon. |
|
@astrofrog I ran into an issue with not being able to apply a subset in this PR. Even before I change the display unit or add both data. The grayed out selector appears and moves when I click and drag but when I let go, nothing happens. EDIT: I just tested other notebooks and subsets work as expected, so it is not a problem with the environment. Does the glue profile viewer work with the display units and subsets? |
|
@javerbukh - I have now merged the two glue PRs. The glue-core changes are in glue-core 1.7.0 while for glue-jupyter the changes are unreleased for now but in the main branch. Could you check whether things in this PR work fine with the latest version of glue-core and latest developer version of glue-jupyter? Subsets work for me now in the glue-jupyter demo notebook but let me know if they still don't work here. |
|
Hi @astrofrog , I upgraded my environment but I'm still not seeing the subset work in the glue-jupyter test case (https://github.com/glue-viz/glue-jupyter/pull/311/files#diff-e542cb99a33595d0713cfa320a27bae9b241a3b602275fe70fb94929a78338c3R37-R59) Screen.Recording.2023-02-03.at.9.32.37.AM.movAnd here is my |
|
Could you check what happens if you add more datapoints? It might be that more than one point has to be selected for the subset to be visible (as it is drawn with lines) |
|
Good call! It does in fact work. Thank you! Screen.Recording.2023-02-03.at.10.07.03.AM.mov |
|
Great! I will go ahead and do a glue-jupyter release when I am at my computer next 😊 |
|
Superseded by #2022 . |
Description
This PR is a proof of concept notebook of unit conversion in specviz. It is a companion to the following PRs in glue-core and glue-jupyter:
This PR should:
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.
triviallabel.CHANGES.rst?