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.

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

Add API to rename subsets #3356

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Dec 13, 2024

Started from #1291 and #2610, I think I've covered all the points raised in #1539 but I'm still doing a bit more work, so opening as draft.

@rosteen rosteen added api API change plugin Label for plugins common to multiple configurations labels Dec 13, 2024
@rosteen rosteen added this to the 4.1 milestone Dec 13, 2024
@rosteen rosteen force-pushed the rename-subsets-api branch 2 times, most recently from e2d811c to 0e8080a Compare December 17, 2024 21:55
@rosteen
Copy link
Collaborator Author

rosteen commented Dec 18, 2024

A couple thoughts as I move this to Ready for Review:

  1. I had initially put _rename_subsets in app.py because I thought that it would touch a lot of things throughout the app, but now those are pretty much handled by broadcasting SubsetRenameMessage. Would it be better to move this to subset_tools.py at this point?

  2. _check_valid_subset_label is currently only set up to Warn, but I coded the warn_if_invalid kwarg as a toggle so that it could potentially return the warning string instead, for live updating a displayed warning in the UI as the label is typed. I wasn't sure if that should be part of this ticket or the UI one.

  3. I started down the path of making the label check case-insensitive, but it seemed like that was going to end up being more confusing for the user, so it's currently case sensitive.

@rosteen rosteen marked this pull request as ready for review December 18, 2024 17:12
Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 58.82353% with 42 lines in your changes missing coverage. Please review.

Project coverage is 87.96%. Comparing base (24c4440) to head (4c431a9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
jdaviz/core/template_mixin.py 33.33% 16 Missing ⚠️
jdaviz/app.py 64.28% 15 Missing ⚠️
jdaviz/core/events.py 53.33% 7 Missing ⚠️
...nfigs/default/plugins/subset_tools/subset_tools.py 80.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3356      +/-   ##
==========================================
- Coverage   88.12%   87.96%   -0.16%     
==========================================
  Files         127      127              
  Lines       19574    19667      +93     
==========================================
+ Hits        17249    17301      +52     
- Misses       2325     2366      +41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kecnry
Copy link
Member

kecnry commented Dec 19, 2024

Noticed a few things (some could be deferred to follow-ups):

  • trying to rename a subset that does not exist raises a cryptic error message, it would be nice to catch this immediately and raise a useful message
  • renamed subsets (I renamed 'Subset 1' to 'Subset blah') no longer appear in the new data-menu/legend or in plot options
  • I think an invalid rename should raise an actual error (at least when triggered from the API - we might want to catch those eventually from the UI and display a message instead of a traceback)
  • renaming something to Subset N can eventually conflict with the default subset created by glue
  • this currently allows naming the subset the same as an existing data-label (and do we need to add checks for loading data that they cannot match any existing subset name either) (ok, it actually didn't, it just raised a warning instead of error - see above)
  • renaming a subset results in duplicate entries in the export plugin
  • update entries in data.meta['_update_live_plugin_results'] so that the live-connections remain intact

jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved
jdaviz/core/template_mixin.py Outdated Show resolved Hide resolved
@javerbukh
Copy link
Contributor

To add to @kecnry's points, I see the old subset name in Line Analysis under Data. That may be because the data was created with Subset 1, but I'm not sure if we keep the name in that case or go through data created with the old name and update that too. That may be a huge headache though so that can be a follow up effort.

@rosteen
Copy link
Collaborator Author

rosteen commented Dec 20, 2024

To add to @kecnry's points, I see the old subset name in Line Analysis under Data. That may be because the data was created with Subset 1, but I'm not sure if we keep the name in that case or go through data created with the old name and update that too. That may be a huge headache though so that can be a follow up effort.

I was planning to leave data entries created with the old subset name as-is, at least for now. I can add some documentation that that's the case (or we can add that once the UI part of this is done). It might not end up being that bad to implement, but I was worried about potential complexities (for example, if doing a simple find replace in the labels of the data objects, what if you're renaming "Subset 1" but there's also a "Subset 10"?).

@kecnry
Copy link
Member

kecnry commented Dec 20, 2024

There is an issue though where the automatic updating spectrum returns when the old spectrum is updated, resulting in duplicated entries. Maybe we can defer the renaming of data-entries, but still update entries in data.meta['_update_live_plugin_results'] so that the live-connections remain intact?

self.app._rename_subset(old_label, new_label)
self._sync_available_from_state()

def rename_selected(self, new_label):
Copy link
Member

Choose a reason for hiding this comment

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

Is this more performant or just a shortcut (in which case I'm not sure if its worth the extra code and test-coverage or not, but don't have a strong opinion)?

Just a note to future selves: eventually we likely will want to migrate this logic into the subset dropdown itself (so these would become subset_tools.subset.rename...) since we're planning to use an editable select component, but I think it still makes sense to expose methods at the top-level in this plugin for convenience, so we can always just move the logic there and then have these methods call that instead.

Copy link
Contributor

@javerbukh javerbukh left a comment

Choose a reason for hiding this comment

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

Besides the points @kecnry brought up and the remaining print statement, this looks good!

@rosteen rosteen modified the milestones: 4.1, 4.2 Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API change plugin Label for plugins common to multiple configurations testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants