Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cubeviz test broken by glue 1.19 #2812

Closed
wants to merge 2 commits into from
Closed

Fix cubeviz test broken by glue 1.19 #2812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2822c9f
Reverts #2809
pllim Apr 16, 2024
0395eda
Do not assume flux comp has spectral_axis
pllim Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions jdaviz/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,17 @@ def to_unit(self, data, cid, values, original_units, target_units):
# gives the units of the values array, which might not be the same
# as the original native units of the component in the data.
if cid.label == "flux":
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)
try:
spec = data.get_object(cls=Spectrum1D)
except Exception:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the bare exception here safe or could it get us until trouble in any way? Can we do something to check in advance instead or check for a specific expected exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original traceback is very confusing so I don't know what is the exact exception to catch here. In general, I think falling back to not using spectral axis should be okay when Spectrum1D conversion fails?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the consequence of the exception being triggered (and returning an empty list) is just that that data entry will not show any valid units to convert?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ask @javerbukh since I am not very sure where the output goes. Either that or it only uses basic equivalencies that might succeed or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used when data with a different spectral axis unit from the display data is visualized simultaneously with the display data in a viewer. For example data 1 with um is loaded and data 2 with AA is added, to_unit will convert the data from data 2 so that it matches the units of data 1. An empty list may result in the data not being visualized at all, so we should check that that actually happens and add a snackbar or other message so the user understands why the data was not added.

That said, if spec = data.get_object(cls=Spectrum1D) is throwing an error, something else seems to be broken and this should just temporarily get us around that issue.

TLDR: This looks like a good temporary fix but we should track the larger issue of why this is throwing an exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just worried that any future bug within get_object will be very hard to track down with this blanket exception 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I close this without merge then, and someone else can pick it up next sprint for a proper fix?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to be RuntimeError... but I'll look into if we can feasilby predict in advance instead of relying on try/except

eqv = []
else:
eqv = u.spectral_density(spec.spectral_axis)
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)
else:
eqv = u.spectral_density(spec.spectral_axis)
else: # spectral axis
eqv = u.spectral()

Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ dependencies = [
"traitlets>=5.0.5",
"bqplot>=0.12.37",
"bqplot-image-gl>=1.4.11",
"glue-core>=1.18.0,!=1.19.0",
"glue-core>=1.18.0",
"glue-jupyter>=0.20",
"echo>=0.5.0",
"ipykernel>=6.19.4",
Expand Down