Centralize data label generation #1672

javerbukh · 2022-09-22T18:06:11Z

Description

This pull request is to address inconsistent data labeling throughout jdaviz.

Currently implemented:

Fixes #727

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?
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-09-22T18:50:43Z

Codecov Report

Base: 87.19% // Head: 87.30% // Increases project coverage by +0.11% 🎉

Coverage data is based on head (128b9f1) compared to base (ac58644).
Patch coverage: 88.75% of modified lines in pull request are covered.

❗ Current head 128b9f1 differs from pull request most recent head e6b515a. Consider uploading reports for the commit e6b515a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1672      +/-   ##
==========================================
+ Coverage   87.19%   87.30%   +0.11%     
==========================================
  Files          95       95              
  Lines       10049     9979      -70     
==========================================
- Hits         8762     8712      -50     
+ Misses       1287     1267      -20

Impacted Files	Coverage Δ
jdaviz/configs/specviz2d/plugins/parsers.py	`33.33% <0.00%> (-2.16%)`	⬇️
jdaviz/configs/specviz/plugins/parsers.py	`88.75% <66.66%> (-0.54%)`	⬇️
jdaviz/configs/cubeviz/plugins/parsers.py	`62.75% <71.42%> (-0.57%)`	⬇️
jdaviz/app.py	`93.91% <98.24%> (+0.85%)`	⬆️
...igs/default/plugins/model_fitting/model_fitting.py	`82.03% <100.00%> (ø)`
jdaviz/configs/imviz/helper.py	`96.66% <100.00%> (-0.02%)`	⬇️
jdaviz/configs/imviz/plugins/parsers.py	`100.00% <100.00%> (ø)`
jdaviz/core/registries.py	`79.68% <0.00%> (-2.60%)`	⬇️
jdaviz/configs/specviz/helper.py	`56.62% <0.00%> (-1.33%)`	⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

jdaviz/configs/imviz/tests/test_parser.py

javerbukh · 2022-09-27T13:35:30Z

jdaviz/configs/specviz2d/plugins/parsers.py

+            app.add_data_to_viewer("spectrum-viewer", data_label)
+
+    else:
+        raise NotImplementedError("Spectrum2d parser only takes a filename")


Noticed that Spectrum2d only takes a filename and does not error gracefully if something else is given.

javerbukh · 2022-09-27T15:02:32Z

@havok2063 Can you please look over this PR and let me know if it satisfies the acceptance criteria of the original issue? Thanks!

kecnry · 2022-09-27T15:50:16Z

Editing the snippet to load data from the default specviz notebook to exclude passing the data label fails with an error saying the extension must be specified, is this expected/intended?

from jdaviz import Specviz
specviz = Specviz()

from astropy.utils.data import download_file
fn = download_file('https://data.sdss.org/sas/dr14/sdss/spectro/redux/26/spectra/0751/spec-0751-52251-0160.fits', cache=True)
specviz.load_spectrum(fn, format="SDSS-III/IV spec")

jdaviz/core/helpers.py

javerbukh · 2022-09-27T19:16:36Z

@kecnry Good catch, definitely not intended. I'll work on fixing that.

jdaviz/app.py

jdaviz/configs/imviz/tests/test_parser.py

pllim · 2022-09-30T16:46:43Z

jdaviz/configs/imviz/tests/test_parser.py

@@ -109,7 +110,7 @@ def test_parse_numpy_array_3d(self, imviz_helper, manual_loop):
        for i in range(n_slices):
            data = imviz_helper.app.data_collection[i]
            comp = data.get_component('DATA')
-            assert data.label == f'my_slices_{i}'
+            assert data.label == (f'my_slices ({i})' if i > 0 else 'my_slices')


I don't know how to feel about this. I don't like the new labeling behavior wrt loading each slice of cube as an image, but we also have Cubeviz for cube, so maybe not a deal breaker...

The user can set their own data label, this is just what the default behavior is. The user can create a for loop and within that set the name "my_slices_i".

This PR doesn't actually introduce that behavior, right? It only changes my_slices_i to my_slices (i) (and excludes the (i) for the first slice to be consistent with other duplicate labels)?

When you load a cube as a series of images, not having number for the first frame seems... wrong.

In line 103:

for i in range(n_slices): imviz_helper.load_data(arr[i, :, :], data_label=f'my_slices_{i}', do_link=False)

would fix that since all data labels would be unique.

jdaviz/configs/imviz/tests/test_parser.py

pllim · 2022-09-30T16:55:42Z

jdaviz/app.py

+
+        number_of_duplicates = 0
+        for label in self.data_collection.labels:
+            if data_label in label:


How robust is this check?

Let's say we already loaded "M31" and "M32" (these are Messier object names). And then we load another unrelated image and be lazy and say data_label="M". Will we detect false positive?

It will, "M" will be loaded as "M (2)". How common would that use case be though? And how detrimental would it be for the user to see "M (2)" as opposed to "M"?

It will be super confusing, so maybe ask the POs.

I agree that there are a lot of cases where this can result in confusing behavior. Can regex help us here (to check if the label is identical except for the possibility of an (N) suffix)? Or maybe for now just check for exact matches since we don't allow deleting the loaded data from the UI itself?

I can see the workflow Pey Lian mentioned happening more often than not. Seeing "M (2)" would be confusing.

Agree that this would be confusing. I like @kecnry 's suggestions.

Thank you all for the feedback! The latest commit uses regex to determine if a data label already exists in the app. It took a while to get that working with all the different variations of data labels in our tests (names with brackets are now my least favorite), but this should now be ready for review!

javerbukh · 2022-10-07T10:57:53Z

We had a meeting about adding benchmarking CI and your number 1 is one of those tickets, so I will not open that can of worms here.

javerbukh · 2022-10-07T15:28:13Z

Test named test_case_that_breaks_return_label in test_app.py has a corresponding github issue #1709 to investigate _get_data_item_by_id() in app.py further.

kecnry

Just a few quick (hopefully) clarifying questions. In general, though, I think we should try to get this in soon and can always iterate going forward - its definitely a big step in the right direction and it is quite difficult to imagine every different case in advance!

jdaviz/app.py

kecnry

Needs a change log before merge, but otherwise I think we should get this in and make tweaks as necessary after it gets some real use. Thanks!

pllim · 2022-10-10T22:42:31Z

Oh, boy. I think there are a bunch of conflicts from merging Brett's PR.

javerbukh · 2022-10-11T12:52:44Z

@pllim fixed!

pllim · 2022-10-11T13:32:01Z

Might have to fix incompatibility with what Brett did.

>       return self._viewer_store[viewer_item['id']]
E       TypeError: 'NoneType' object is not subscriptable

javerbukh · 2022-10-11T13:37:02Z

@bmorris3, do any of these errors look familiar to you?

bmorris3 · 2022-10-11T14:02:25Z

@javerbukh I'm not sure but it looks a bit like something went wrong in the rebase? For example, this change seems not to know about what's on main. Same here: main branch=new but this branch=old

jdaviz/app.py

jdaviz/configs/imviz/helper.py

jdaviz/configs/imviz/tests/test_parser.py

pllim

Some minor comments.

jdaviz/app.py

jdaviz/tests/test_app.py

jdaviz/configs/imviz/tests/test_parser.py

github-actions bot added the cubeviz label Sep 22, 2022

javerbukh force-pushed the default_data_labels branch from 64fdad8 to 1dfe756 Compare September 23, 2022 14:03

javerbukh added the imviz label Sep 23, 2022

javerbukh added this to the 2.11 milestone Sep 23, 2022

javerbukh force-pushed the default_data_labels branch from dac9b24 to e455196 Compare September 26, 2022 20:06

javerbukh added specviz specviz2d labels Sep 27, 2022

javerbukh commented Sep 27, 2022

View reviewed changes

jdaviz/configs/imviz/tests/test_parser.py Outdated Show resolved Hide resolved

javerbukh commented Sep 27, 2022

View reviewed changes

javerbukh marked this pull request as ready for review September 27, 2022 13:36

javerbukh requested review from duytnguyendtn, rosteen, ojustino, pllim and kecnry as code owners September 27, 2022 13:36

javerbukh requested a review from havok2063 September 27, 2022 15:02

kecnry reviewed Sep 27, 2022

View reviewed changes

jdaviz/core/helpers.py Outdated Show resolved Hide resolved

kecnry mentioned this pull request Sep 28, 2022

spectral-extraction: support import/export trace via glue-astronomy #1556

Merged

11 tasks

pllim reviewed Sep 29, 2022

View reviewed changes

jdaviz/app.py Outdated Show resolved Hide resolved

javerbukh force-pushed the default_data_labels branch from 3acde8e to c335a05 Compare September 29, 2022 19:26

javerbukh requested review from kecnry and pllim September 29, 2022 21:44

pllim reviewed Sep 30, 2022

View reviewed changes

jdaviz/configs/imviz/tests/test_parser.py Show resolved Hide resolved

pllim reviewed Sep 30, 2022

View reviewed changes

jdaviz/configs/imviz/tests/test_parser.py Outdated Show resolved Hide resolved

pllim reviewed Sep 30, 2022

View reviewed changes

Jdaviz-Triage-Bot mentioned this pull request Oct 7, 2022

Adding data with the same label causes a UserWarning #1709

Closed

kecnry reviewed Oct 10, 2022

View reviewed changes

jdaviz/app.py Outdated Show resolved Hide resolved

jdaviz/app.py Show resolved Hide resolved

kecnry approved these changes Oct 10, 2022

View reviewed changes

kecnry added the Ready for final review label Oct 10, 2022

javerbukh requested a review from pllim October 10, 2022 17:26

rosteen modified the milestones: 2.11, 3.1 Oct 10, 2022

Centralize data label generation

c946d8f

javerbukh force-pushed the default_data_labels branch from 128b9f1 to c946d8f Compare October 11, 2022 12:52

javerbukh requested a review from bmorris3 as a code owner October 11, 2022 12:52

javerbukh added 2 commits October 11, 2022 08:54

Remove unused tests

b15c631

Re-add code that was removed during rebase

2e078d6

Undo rebase errors

b0f76dd

bmorris3 approved these changes Oct 11, 2022

View reviewed changes

jdaviz/app.py Outdated Show resolved Hide resolved

jdaviz/configs/imviz/helper.py Outdated Show resolved Hide resolved

jdaviz/configs/imviz/tests/test_parser.py Outdated Show resolved Hide resolved

pllim reviewed Oct 11, 2022

View reviewed changes

jdaviz/app.py Outdated Show resolved Hide resolved

jdaviz/app.py Outdated Show resolved Hide resolved

jdaviz/tests/test_app.py Outdated Show resolved Hide resolved

pllim reviewed Oct 11, 2022

View reviewed changes

jdaviz/configs/imviz/tests/test_parser.py Outdated Show resolved Hide resolved

javerbukh added 2 commits October 11, 2022 13:00

Address review comments

b1f0672

Remove comment

e6b515a

javerbukh merged commit 3d4157f into spacetelescope:main Oct 11, 2022

javerbukh deleted the default_data_labels branch October 11, 2022 17:52

bmorris3 mentioned this pull request Oct 13, 2022

Label-based lookups in [add/remove]_data_from_viewer #1725

Merged

9 tasks

rosteen mentioned this pull request Nov 1, 2022

Warn/error when adding data with conflicting labels #116

Closed

bmorris3 mentioned this pull request Nov 10, 2022

Towards Roman Level 2 ASDF data product support #1822

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralize data label generation #1672

Centralize data label generation #1672

javerbukh commented Sep 22, 2022 •

edited by pllim

Loading

codecov bot commented Sep 22, 2022 •

edited

Loading

javerbukh Sep 27, 2022

javerbukh commented Sep 27, 2022

kecnry commented Sep 27, 2022

javerbukh commented Sep 27, 2022

pllim Sep 30, 2022

javerbukh Oct 3, 2022

kecnry Oct 3, 2022

pllim Oct 3, 2022 •

edited

Loading

javerbukh Oct 4, 2022

pllim Sep 30, 2022

javerbukh Oct 3, 2022

pllim Oct 3, 2022

javerbukh Oct 3, 2022

kecnry Oct 3, 2022

camipacifici Oct 3, 2022

larrybradley Oct 3, 2022

javerbukh Oct 3, 2022

javerbukh commented Oct 7, 2022 •

edited

Loading

javerbukh commented Oct 7, 2022

kecnry left a comment

kecnry left a comment

pllim commented Oct 10, 2022

javerbukh commented Oct 11, 2022

pllim commented Oct 11, 2022

javerbukh commented Oct 11, 2022

bmorris3 commented Oct 11, 2022

pllim left a comment

Centralize data label generation #1672

Centralize data label generation #1672

Conversation

javerbukh commented Sep 22, 2022 • edited by pllim Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Sep 22, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

javerbukh commented Sep 27, 2022

kecnry commented Sep 27, 2022

javerbukh commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javerbukh commented Oct 7, 2022 • edited Loading

javerbukh commented Oct 7, 2022

kecnry left a comment

Choose a reason for hiding this comment

kecnry left a comment

Choose a reason for hiding this comment

pllim commented Oct 10, 2022

javerbukh commented Oct 11, 2022

pllim commented Oct 11, 2022

javerbukh commented Oct 11, 2022

bmorris3 commented Oct 11, 2022

pllim left a comment

Choose a reason for hiding this comment

javerbukh commented Sep 22, 2022 •

edited by pllim

Loading

codecov bot commented Sep 22, 2022 •

edited

Loading

pllim Oct 3, 2022 •

edited

Loading

javerbukh commented Oct 7, 2022 •

edited

Loading