Added ability to convert units in profile viewer #2296
Conversation
|
As mentioned in glue-viz/glue-jupyter#311 I am planning on making the unit conversion pluggable/swappable. More soon! |
|
When you have coded it, I am gonna throw in some magnitudes to make your life interesting, maybe. 😉 |
| the ability to select units for the x and y axes is only available in the profile viewer. | ||
|
|
||
| Data components can be assigned units as a string (or `None` to indicate no known units). | ||
| By default, glue uses `astropy.units <https://docs.astropy.org/en/stable/units/index.html>`_ package |
There was a problem hiding this comment.
Do you just wanna use intersphinx here?
| to theb carry out unit conversions based on these units. However, it is possible to customize the | ||
| unit conversion machinery, either to use a different unit transformation machinery, or to specify | ||
| e.g. equivalencies in the astropy unit conversion. To customize the unit conversion behavior, you | ||
| will need to define a unit converter as shown below:: |
There was a problem hiding this comment.
So, this is a way to bypass built-in astropy equivalencies? 👀
|
p.s. I think at its current state, this PR broke Specviz parser. I had this installed and was doing something else and it crashed. The error went away when I uninstalled this branch and reinstalled glue-core 1.3.0 |
astrofrog
force-pushed
the
units
branch
2 times, most recently
from
2ff1c2b to
7696e13
Compare
astrofrog
changed the title
… unit conversion classes
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
|
I will go ahead and merge this, but @dhomeier feel free to take a look at this anyway if you have time and let me know if you spot anything that needs fixing! |
| .. note:: Support for automatic unit conversion in glue is experimental - at the moment | ||
| the ability to select units for the x and y axes is only available in the profile viewer. | ||
|
|
||
| Data components can be assigned units as a string (or `None` to indicate no known units). |
There was a problem hiding this comment.
This breaks backward compatibility -- See failures in https://github.com/spacetelescope/jdaviz/actions/runs/4053913069/jobs/6975103932
There was a problem hiding this comment.
Thanks, will have a think if there is a way we can avoid breaking backward compatibility
| axis_values = converter.to_unit(self.viewer_state.reference_data, | ||
| self.viewer_state.x_att, axis_values, | ||
| self.viewer_state.x_display_unit) |
There was a problem hiding this comment.
Something does not look quite right here. Why does this use reference data but the next line use input data? I tried changing this but I cannot make it work.
I am seeing over at spacetelescope/jdaviz#2048 that when I add a second spectrum with GHz (first spectrum is loaded in micron and plot is showing micron), the second data plots GHz values as if they are micron without any conversion.
The inputs to custom Jdaviz converter registers as such:
# Signature: to_unit(self, data, cid, values, original_units, target_units)
data = Jy_um # But I am loading mJy_GHz
cid = World 0
values = [142758.31333333 123202.38 108358.71975904 96707.24451613 87318.19165049 79590.9180531 73120.11170732 67622.35894737 62893.52265734 58782.83490196]
original_units = um # This is wrong, should be GHz
target_units = umIt is also very confusing that the custom converter takes one more arg than what is really used over here.
This adds initial support for unit conversion in the profile viewer, as a test case that is also useful for jdaviz.
Remaining TODOs: