toggle flux <-> surface brightness units #2781
Conversation
github-actions
bot
added
specviz
plugin
gibsongreen
force-pushed
the
flux_sb_switch
branch
from
f326273 to
0c3a2a6
Compare
|
@gibsongreen - at first glance, I think |
Line 112 in 0c3a2a6 Hey there, I believe that this is relating to the custom equivalency. I use the data item for any spectra that goes through spectral extraction. This remains as just |
jdaviz/app.py
Outdated
| ] | ||
| + [ | ||
| 'Jy / sr', 'mJy / sr', 'uJy / sr', 'MJy / sr', | ||
| 'W / (m2 Hz sr)', | ||
| 'eV / (s m2 Hz sr)', | ||
| 'erg / (s cm2 sr)', | ||
| 'erg / (s cm2 Angstrom sr)', 'erg / (s cm2 Hz sr)', | ||
| 'ph / (s cm2 Angstrom sr)', 'ph / (s cm2 Hz sr)' |
There was a problem hiding this comment.
@astrofrog I do think this update to the equivalent units set needed to be addressed, however the tracebacks regarding IncompatibleAttribute and UnitConversionError aren't resolved from this addition.
Both tracebacks last jdaviz trigger for this diff is in unit_conversions.py at the two statements: self.spectrum_viewer.state.y_display_unit = str(spec_units). Both occur whether or not I use astropy's custom equivalencies. Everything updates in the viewer/display_units, but the tracebacks proceed.
Also, it still may be needed to determine, but with this change, y-limits on the profile viewer are not updating with this change (but this may change if the tracebacks are resolved).
|
I had a chance to investigate this and am a little confused by the current implementation of some of the unit-related functionality which might explain the issue here. I tried out the cubeviz notebook with this branch, and currently the only available unit under surface brightness is MJy/sr, so it makes sense it will error if you try and change it to anything else currently. This appears to be due to the fact that the choices for the units are set in choices = create_flux_equivalencies_list(y_u, x_u)
# ensure that original entry is in the list of choices
if not np.any([y_u == u.Unit(choice) for choice in choices]):
choices = [y_unit] + choicesHowever def create_flux_equivalencies_list(flux_unit, spectral_axis_unit):
"""Get all possible conversions for flux from current flux units."""
if ((flux_unit in (u.count, (u.MJy / u.sr), u.dimensionless_unscaled))
or (spectral_axis_unit in (u.pix, u.dimensionless_unscaled))):
return []so then only MJy/sr is in the list of choices. That seems like the cause of the issue here? What I'm also confused about is why the units in the list are updated when |
|
Ok I think I see why you are listening to changes in y_display_unit - it's because you are providing another layer of unit choices on top of the one actually available under the scenes in glue. It looks like if I access the glue viewer behind the scenes I can change the units to MJy instead of MJy/sr and the plot updates without any issues:
although this doesn't actually seem to change anything in the plot, which doesn't seem right. Furthermore, if I convert it back to MJy / sr it raises an error, I think because the following does not get executed: elif '_pixel_scale_factor' in spec.meta:
# custom equivalency
eqv = [(u.MJy / u.sr,
u.MJy,
lambda x: (x * spec.meta['_pixel_scale_factor']),
lambda x: x)]To be clear, at this point I have not even created a subset, but this should in principle work. So I guess the question is, what exactly is |
|
Ok so it appears that |
jdaviz/app.py
Outdated
| spec = data.get_object(cls=Spectrum1D) | ||
| if len(values) == 2: | ||
| # Need this for setting the y-limits | ||
| spec_limits = [spec.spectral_axis[0].value, spec.spectral_axis[-1].value] | ||
| eqv = u.spectral_density(spec_limits*spec.spectral_axis.unit) | ||
|
|
||
| elif '_pixel_scale_factor' in spec.meta: |
There was a problem hiding this comment.
Check if this condition ever gets triggered for you - it does not appear to be to me, which I think is the root cause of the issues you are seeing.
There was a problem hiding this comment.
The pixel_scale_factor is set to spectrum that pass through Cubeviz's collapse function. Another effort that is being worked on is to have all spectra go through the collapse function instead of glues auto-collapse. When this is implemented all spectra that appear in the spectrum-viewer will have this pixel scale factor.
For the purpose of this PR, we do need to create a spatial subset, and pass it through spectral extraction in order to have a pixel scale factor stored. Once this is completed, and the translation is attempted, it will enter this elif statement. It does also mean that for now, only this one spectrum will have translate in the viewer.
There was a problem hiding this comment.
I think the issue with this PR is that even if one layer does have this property defined, you will still see an error if any layers are not eg a subset.
There was a problem hiding this comment.
In tagup today, Kyle suggested something similar. I plan on testing this PR where I only use the spectra that have passed through the collapse function (either removing the others or rebasing on Kyle's PR). I will get back to you soon on the results!
There was a problem hiding this comment.
Update: I removed all data items from the data collection excluding the spectrum that went through the collapse function. I still am encountering the same errors.
I also rebased onto this branch: #2827 which passes all spectra through the collapse function and I also still encounter the tracebacks
|
A related issue is that the following also fails for me:
even though there clearly is a spectrum shown in the viewer, just not one made from a subset. |
gibsongreen
force-pushed
the
flux_sb_switch
branch
from
83331ce to
1abe0bb
Compare
| def create_flux_equivalencies_list(flux_unit, spectral_axis_unit): | ||
| """Get all possible conversions for flux from current flux units.""" | ||
| if ((flux_unit in (u.count, (u.MJy / u.sr), u.dimensionless_unscaled)) |
There was a problem hiding this comment.
@astrofrog I had made this update to remove u.MJ / u.sr, so the list of surface brightness units should populate with all available options.
| 'Jy', 'mJy', 'uJy', | ||
| include_prefix_units=True, equivalencies=eqv))) | ||
| + [ | ||
| 'Jy', 'mJy', 'uJy', 'MJy', | ||
| 'W / (m2 Hz)', 'W / (Hz m2)', # Order is different in astropy v5.3 |
There was a problem hiding this comment.
do we know which version is for 5.3+? #2827 will bump astropy requirement beyond 5.3 so maybe at that point we can clean this up?
There was a problem hiding this comment.
I've found the order with which units are displayed/stored has significancy in the conversion and in causing tracebacks, I would like this to also to be cleaner. The "W / (Hz m^2)" is the unit 5.3+.
There was a problem hiding this comment.
nothing to do here for now. Once rebased on top of this, #2827 can always remove the unneeded line.
jdaviz/app.py
Outdated
| lambda x: (x * spec.meta['_pixel_scale_factor']), | ||
| lambda x: x)] | ||
|
|
||
| # below is the generalized version |
There was a problem hiding this comment.
do we not need the generalized version already?
There was a problem hiding this comment.
I believe that the equivalency that gets passed to:
Line 180 in 1799309
Will be used irregardless of what the actual units for
original_units and target_units. The generalized version should be used, but it results in more issues. Also, for this commit I was using the specific equivalency for the sake of simplicity in diagnosing bugs with Tom.
There was a problem hiding this comment.
ok, if the other one handles the prefixes, etc, then can we just remove the commented code?
| self.flux_unit = UnitSelectPluginComponent(self, | ||
| items='flux_unit_items', | ||
| selected='flux_unit_selected') |
There was a problem hiding this comment.
Should we have separate flux_unit and sb_unit and the flux_or_sb switch just changes which is visible (instead of swapping out all the choices). Or, alternatively, should this be renamed to flux_or_sb_unit?
Either way, we should also think about how this affects specviz where we probably will not allow translating since we won't know the scale factor, but the input spectra could either be in flux or surface brightness 🤔
There was a problem hiding this comment.
I have added a return statement at the beginning of:
It is also disabled through the vue file. You can still 'see' the bool but it doesn't do anything (before if you tried you would get the ValueError thrown that there isn't a scale factor). If there are additional ways to disable between configs I will make sure to include them in this PR. A following comment addresses flux/sb_unit| self.flux_or_sb = SelectPluginComponent(self, | ||
| items='flux_or_sb_items', | ||
| selected='flux_or_sb_selected', | ||
| manual_options=['Surface Brightness', 'Flux']) | ||
|
|
||
| @property | ||
| def user_api(self): |
There was a problem hiding this comment.
we'll probably want to expose flux_or_sb through the user API here. And if we decide to rename flux_unit, then we'll need to account for backwards compatibility (there are other cases where we do that - see width/continuum_width in line_analysis, for example).
There was a problem hiding this comment.
My following commit, the flux_unit and sb_unit will be included with this same methodology as line_analysis. Thank you for pointing me in the right direction! I am trying to finalize the case where the units have been translated, and after that, the units are converted (which differentiates from the data collection's data item).
| @@ -52,6 +53,10 @@ class UnitConversion(PluginTemplateMixin): | |||
| flux_unit_items = List().tag(sync=True) | |||
| flux_unit_selected = Unicode().tag(sync=True) | |||
|
|
|||
| show_translator = Bool(False).tag(sync=True) | |||
There was a problem hiding this comment.
would this ever change during an active session or is this just to disable this capability if this PR is merged before the 3.10 release?
There was a problem hiding this comment.
I am using show_translator for disabling right now. It does have a secondary purpose. When changed to true, an observe is triggered and the correct physical type is selected in the dropdown to start. It may not be best practices to set it this way, if this is done differently and correctly elsewhere I will make the change.
There was a problem hiding this comment.
ok, if its not easy to remove for now, then we can do this in the follow-up as well. I suspect we'll either want to listen to AddData events on the spectrum viewer or perhaps just make a call from the parser to set the initial value
EDIT: we'll be removing this in a follow-up PR.
gibsongreen
force-pushed
the
flux_sb_switch
branch
2 times, most recently
from
3c27481 to
c158bcc
Compare
gibsongreen
force-pushed
the
flux_sb_switch
branch
from
63be01f to
8b1ced7
Compare
| * ``flux_unit`` (:class:`~jdaviz.core.template_mixin.SelectPluginComponent`): | ||
| * ``flux_or_sb_unit`` (:class:`~jdaviz.core.template_mixin.SelectPluginComponent`): |
There was a problem hiding this comment.
I think this requires some thought/discussion. Do we want a single select/dropdown for flux_or_sb_unit that changes its choices based on flux_or_sb or separate flux_unit and sb_unit and just toggle which is shown - or maybe even show both (and then have changing one automatically update the other to match).
I'm leaning towards the second option since we'll eventually probably want to bring this to Imviz where there won't be a flux_or_sb selection, but will want to have a selection for surface brightness units which will affect the resulting flux units (from aperture photometry, for example).
EDIT: we'll be addressing this in a follow-up PR.
jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py
Outdated
Show resolved
Hide resolved
| return PluginUserApi(self, expose=('spectral_unit', 'flux_unit')) | ||
| return PluginUserApi(self, expose=('spectral_unit', 'flux_or_sb_unit', 'flux_or_sb')) |
There was a problem hiding this comment.
let's only expose flux_or_sb for cubeviz. Depending on the decision on my point above regarding flux_or_sb_unit, we may or may not want to expose that for specviz/imviz as well, but if that would require extra work beside exposing here and an if-statement in the vue file, let's create a separate ticket for flux unit conversion in specviz and imviz.
There was a problem hiding this comment.
On rebasing this PR: #2846
spectral_unit is now only exposed. Unexposing flux_or_sb_unit is why this test is failing, I know how to use _obj/unexposed functions, I am unsure how to use this class instance if it isn't exposed. I might expose it temporarily just so I can run the CI and know if all tests are passing:
There was a problem hiding this comment.
This PR can re-expose whatever is needed. We just wanted to hide them for the 3.10 release so we could have flexibility to decide on the API still.
There was a problem hiding this comment.
Bearing CI passing, I will move on to adding the flux_unit and sb_unit and toggling between them
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2781 +/- ##
==========================================
+ Coverage 88.90% 88.94% +0.03%
==========================================
Files 111 111
Lines 17148 17201 +53
==========================================
+ Hits 15246 15299 +53
Misses 1902 1902 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for all the hard work on this one! This works for me under the assumption that:
- Cubeviz spectral extraction through plugin #2827 will be coming soon (this PR intentionally leaves things in an "awkward" state where only extracted spectra are handled... if for some reason Cubeviz spectral extraction through plugin #2827 is abandoned, we would need to either revert this or change the implementation)
- test coverage is increased before merge to trigger this logic (although that could also be a follow-up if it'll be significantly easier after Cubeviz spectral extraction through plugin #2827 exists)
- we have tickets for follow-up efforts to:
- split the SB and flux dropdowns into two and enable for non-cubeviz configs 🐱
- remove the
show_translatortraitlet entirely
- changelog entry is added (as the PR stands now, the UI does not expose this, but the API does, so let's at least create an entry that links back to this PR and we'll likely append other PR numbers to it as the rest is exposed)
kecnry
requested review from
rosteen,
javerbukh,
pllim,
bmorris3,
haticekaratay and
cshanahan1
as code owners
…e, sb/flux dropdown updating with correct units
…st from sb conversion test
gibsongreen
force-pushed
the
flux_sb_switch
branch
from
8387d03 to
d4eace5
Compare
Description
This pull request is to address adding a UI toggle for translating between surface brightness and flux within the unexposed Unit Conversion Plugin.
Main Notes:
data_itemindata_collectionis still used and has values in MJy/sr (data_collection doesn't update).UnitConversionError: 'MJy' (spectral flux density) and 'MJy / sr' are not convertibleandIncompatibleAttribute: flux.PR (buggy) behavior:
Screen.Recording.2024-04-25.at.4.48.56.PM.mov
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.