fix event handling for choosing config from launcher #2417

kecnry · 2023-09-01T16:57:08Z

Description

This pull request fixes the event handling on the click event in the launcher.

Note: v3.6 does not have this full version of the launcher, so this should not need to be backported.

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

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.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

kecnry · 2023-09-01T17:06:34Z

jdaviz/core/launcher.vue

-        @click="launch_config(config)"
+        @click="launch_config({config: config})"


I have no idea how this ever worked... I'm guessing it was changed at one point but anyone testing with an editable install was not getting the latest version 🤷. Reviewers - you may need a fresh env to force the templates to be updated in your installation.

codecov · 2023-09-01T17:14:27Z

Codecov Report

Patch coverage is 50.00% of modified lines.

❗ Current head 87264e0 differs from pull request most recent head 51e65ef. Consider uploading reports for the commit 51e65ef to get more accurate results

Files Changed	Coverage
jdaviz/core/launcher.py	`50.00%`

📢 Thoughts on this report? Let us know!.

pllim

Diff looks reasonable. Works when I tried this patch on Windows.

rosteen · 2023-09-05T15:30:30Z

This works when I run jdaviz from the command line and then immediately click a config. However, if I load a file and then click the config it highlights as relevant, nothing happens still (for example loading an x1d file and then clicking Specviz). There must still be a bug in that logic.

pllim · 2023-09-05T15:33:24Z

@rosteen , that's weird. Worked for me when I tested with Imviz (load file and then click with Imviz the only thing enabled). I used Windows 10 + Chrome, but I did not have Notebook 7.

More info: It was standalone and I installed using pip install . on the branch checkout after rebasing on upstream/main.

rosteen · 2023-09-05T15:38:24Z

@rosteen , that's weird. Worked for me when I tested with Imviz (load file and then click with Imviz the only thing enabled). I used Windows 10 + Chrome, but I did not have Notebook 7.

It's nothing to do with Notebook 7, I see the same thing with 6.x. Looks like it's actually Specviz-specific, I see Specviz failing to load with multiple input file (so it's not just a bad file), but I do actually see Imviz working on my machine as well with an i2d file selected.

rosteen · 2023-09-05T15:43:17Z

Maybe there's a bug in the way Specviz.load_data() is getting called? I know that originally Specviz used load_spectra and load_data there is more recent 🤔

rosteen · 2023-09-05T15:45:40Z

Maybe there's a bug in the way Specviz.load_data() is getting called? I know that originally Specviz used load_spectra and load_data there is more recent 🤔

I think this is it, looks like Specviz.load_data doesn't have a **kwargs input handler and probably chokes on the 'verbosity' and 'history_verbosity' keywords input from jdaviz_cli.ipynb.

pllim · 2023-09-05T15:45:48Z

You are right. I can reproduce this for Specviz only.

pllim · 2023-09-05T15:49:26Z

Loading the same file directly in notebook using specviz.load_data(...) works, so the bug must be in the launch somewhere.

pllim · 2023-09-05T15:57:49Z

If I do this directly outside of voila, I get an error:

>>> from jdaviz.core.launcher import open
>>> filename = "jw01236011001_03101_00001_mirifulong_x1d.fits"
>>> open(filename)
IORegistryError: Format is ambiguous - options are: JWST x1d, JWST x1d MIRI MRS

This same file loads when I call specviz.load_data(filename).

pllim · 2023-09-05T16:03:12Z

I don't understand why the launcher insists on using hdul from

jdaviz/jdaviz/core/data_formats.py

Line 270 in 1577249

return ([config], hdul)

and not passing in the original input filename. That function in data_formats.py is bypassing real parser logic, tricking Specviz into thinking it is loading a SpectrumList, and then Specviz crashes.

rosteen

Specviz now loads data successfully like the rest of the configs, approved. Thanks!

rosteen · 2023-09-05T17:16:45Z

jdaviz/core/launcher.py

+        try:
+            viz_helper.load_data(data, **kwargs)
+        except IORegistryError:
+            if filepath is None:
+                raise
+            viz_helper.load_data(filepath, **kwargs)


Not the prettiest thing I've ever seen but I don't hate it. And it works, which is the important thing!

agreed... hopefully we can revert this down the road, but that may require some significant refactoring 🐱

kecnry added the bug Something isn't working label Sep 1, 2023

kecnry added this to the 3.7 milestone Sep 1, 2023

fix event handling for choosing config from launcher

87264e0

kecnry force-pushed the fix-launcher-click branch from 3fc7e56 to 87264e0 Compare September 1, 2023 16:58

kecnry marked this pull request as ready for review September 1, 2023 16:58

kecnry requested review from rosteen, javerbukh, pllim, bmorris3, haticekaratay and cshanahan1 as code owners September 1, 2023 16:58

kecnry mentioned this pull request Sep 1, 2023

provide access to API from show_launcher #2418

Closed

9 tasks

kecnry commented Sep 1, 2023

View reviewed changes

pllim approved these changes Sep 1, 2023

View reviewed changes

Jdaviz-Triage-Bot mentioned this pull request Sep 1, 2023

Launch config auto-detect does not recognize HST/WFC3 UVIS FLC image #2419

Open

kecnry added the Ready for final review label Sep 5, 2023

filepath fallback when object from config-detection fails to load

51e65ef

rosteen approved these changes Sep 5, 2023

View reviewed changes

kecnry enabled auto-merge September 5, 2023 17:23

kecnry merged commit 1612c33 into spacetelescope:main Sep 5, 2023
11 of 12 checks passed

kecnry deleted the fix-launcher-click branch September 5, 2023 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix event handling for choosing config from launcher #2417

fix event handling for choosing config from launcher #2417

kecnry commented Sep 1, 2023 •

edited

Loading

kecnry Sep 1, 2023

codecov bot commented Sep 1, 2023 •

edited

Loading

pllim left a comment

rosteen commented Sep 5, 2023 •

edited

Loading

pllim commented Sep 5, 2023 •

edited

Loading

rosteen commented Sep 5, 2023

rosteen commented Sep 5, 2023

rosteen commented Sep 5, 2023

pllim commented Sep 5, 2023

pllim commented Sep 5, 2023

pllim commented Sep 5, 2023

pllim commented Sep 5, 2023 •

edited

Loading

rosteen left a comment

rosteen Sep 5, 2023

kecnry Sep 5, 2023

		@click="launch_config(config)"
		@click="launch_config({config: config})"

fix event handling for choosing config from launcher #2417

fix event handling for choosing config from launcher #2417

Conversation

kecnry commented Sep 1, 2023 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

kecnry Sep 1, 2023

Choose a reason for hiding this comment

codecov bot commented Sep 1, 2023 • edited Loading

Codecov Report

pllim left a comment

Choose a reason for hiding this comment

rosteen commented Sep 5, 2023 • edited Loading

pllim commented Sep 5, 2023 • edited Loading

rosteen commented Sep 5, 2023

rosteen commented Sep 5, 2023

rosteen commented Sep 5, 2023

pllim commented Sep 5, 2023

pllim commented Sep 5, 2023

pllim commented Sep 5, 2023

pllim commented Sep 5, 2023 • edited Loading

rosteen left a comment

Choose a reason for hiding this comment

rosteen Sep 5, 2023

Choose a reason for hiding this comment

kecnry Sep 5, 2023

Choose a reason for hiding this comment

kecnry commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 1, 2023 •

edited

Loading

rosteen commented Sep 5, 2023 •

edited

Loading

pllim commented Sep 5, 2023 •

edited

Loading

pllim commented Sep 5, 2023 •

edited

Loading