POC: Notebook showing glue unit conversion in Specviz #2022
Conversation
Start changing unit conversion plugin Continue hacking
|
Does this supersede #1398 ? |
| @@ -4,6 +4,7 @@ | |||
| description='Convert the spectral flux density and spectral axis units.' | |||
| :link="'https://jdaviz.readthedocs.io/en/'+vdocs+'/'+config+'/plugins.html#unit-conversion'" | |||
| disabled_msg="This plugin is temporarily disabled. Effort to improve it is being tracked at GitHub Issue 1972." | |||
There was a problem hiding this comment.
(although we probably won't want to actually merge this until the plugin itself is updated)
| disabled_msg="This plugin is temporarily disabled. Effort to improve it is being tracked at GitHub Issue 1972." |
|
@pllim Yes it does, I will close that PR and link to this one. |
| " all_units = []\n", | ||
| " for unit in unit_list: \n", | ||
| " try:\n", | ||
| " u.Unit(unit)\n", |
There was a problem hiding this comment.
You don't have to keep calling u.Unit(unit). It has overhead. Save the result if successful and reuse it throughout the rest of the function.
| " u.Unit(unit)\n", | |
| " cur_unit = u.Unit(unit)\n", |
| " # warnings.warn(f\"{unit}: {te}\")\n", | ||
| " continue\n", | ||
| " if u.Unit(unit) == u.Unit(\"Angstrom\"):\n", | ||
| " all_units.append(u.Unit(unit.name))\n", |
There was a problem hiding this comment.
I don't get this: So if it is Angstrom, you want the Unit object but if not, then you want the unit string? Then won't your output list have mixed object/str elements? Why do you need such a behavior?
| " \n", | ||
| " def create_flux_equivalencies_list(self, data, cid, units):\n", | ||
| " spec = data.get_object(cls=Spectrum1D)\n", | ||
| " equivalencies = u.spectral_density(np.sum(spec.spectral_axis))\n", |
There was a problem hiding this comment.
| " equivalencies = u.spectral_density(np.sum(spec.spectral_axis))\n", | |
| " equivalencies = u.spectral_density(spec.spectral_axis)\n", |
| "from glue.core.units import unit_converter\n", | ||
| "\n", | ||
| "@unit_converter('custom-jdaviz')\n", | ||
| "class UnitConverterWithSpectral:\n", |
There was a problem hiding this comment.
I feel like this is overly complicated but I also don't know how glue set up the unit support. The tests at https://github.com/glue-viz/glue/blob/main/glue/core/tests/test_units.py are minimal and does not tell me much when it comes to equivalencies.
In principle, astropy.units should do the conversion for you out of the box. All we need to do is detect if it is spectral axis or flux:
- spectral axis: use
u.spectral() - flux: use
u.spectral_densitry(spectral_axis)
There should be no need to hardcode so many things. The Unit Conversion plugin already should limit what the users can choose from, so probably okay to assume the input is well-behaved.
I might be missing something here though. Please help me understand. Thanks!
There was a problem hiding this comment.
I spent about an hour trying to move the code to use something like this and with those changes I was unable to get the data loading into the spectrum viewer at all! I'm definitely still in the learning stages as well with glue unit conversion so it might just be my lack of familiarity. I would welcome you trying to get it working if you can!
There was a problem hiding this comment.
Okay, lemme see. Maybe I will convince my Past Self that I was wrong lol.
There was a problem hiding this comment.
@javerbukh , does javerbukh#14 work as intended or did I miss the mark? 😬
|
We're moving future efforts towards the actual development in jdaviz rather than wrapping up the fine details in this notebook. |
Description
This pull request is to add a concept notebook that shows how glue unit conversion can be implemented in Jdaviz.
Fixes #
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.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.