Skip to content

Don't add actual subsets to CubevizProfileView#3626

Merged
kecnry merged 5 commits into
spacetelescope:mainfrom
astrofrog:no-subsets-in-cubeviz-profile
Jun 17, 2025
Merged

Don't add actual subsets to CubevizProfileView#3626
kecnry merged 5 commits into
spacetelescope:mainfrom
astrofrog:no-subsets-in-cubeviz-profile

Conversation

@astrofrog
Copy link
Copy Markdown
Collaborator

Description

While I don't understand the motivations behind this, it seems the cubeviz profile viewer doesn't actually use glue subsets and instead uses extracted datasets and pretends they are subsets. However, because of the way glue works, all subsets that are defined automatically get added to all datasets, and if a dataset is in a viewer, subsets will get added automatically. This means that if one had a cube loaded in cubeviz and had extracted say 10 subsets, there would be 11 datasets in the cubeviz profile viewer plus 110 subsets. The subsets were all set to be not visible, but having them in the viewer still slows things down a lot as they still all have a layer artist and state instantiated in case the visibility changes.

The simple fix is to simply not accept any subsets in the cubeviz profile viewer. This is valid usage - add_data and add_subset can just return without adding data or subsets.

With 10 subsets, this speeds up cubeviz by a factor of 9x (in addition to the new echo 0.11 release)

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. Bugfix milestone also needs an accompanying backport label.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (5f9237d) to head (b140ad4).
Report is 3 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff      @@
##   main   #3626   +/-   ##
============================
============================

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kecnry
Copy link
Copy Markdown
Member

kecnry commented Jun 13, 2025

While I don't understand the motivations behind this, it seems the cubeviz profile viewer doesn't actually use glue subsets and instead uses extracted datasets and pretends they are subsets.

This was because we needed more advanced options for how to handle the extraction than simply pulling the pixels from within the max. Ideally we should probably move away from the profile viewer and use a scatter viewer instead, right?

This PR seems to break creating spectral subsets (xrange) in the profile viewer itself. Is there anyway to restore that functionality and only block adding the profiles of spatial subsets created in the cube viewers?

@astrofrog astrofrog force-pushed the no-subsets-in-cubeviz-profile branch from 6abf102 to 1e34008 Compare June 13, 2025 12:45
@astrofrog
Copy link
Copy Markdown
Collaborator Author

@kecnry I've pushed a fix, the easiest way at the moment is to check if the subset state is derived based on one or two attributes - if it's based on two we know it was made in the image viewer.

@rosteen rosteen modified the milestones: 4.2.3, 4.2.4 Jun 16, 2025
@astrofrog astrofrog force-pushed the no-subsets-in-cubeviz-profile branch from 1e34008 to c391e54 Compare June 17, 2025 13:38
@astrofrog astrofrog force-pushed the no-subsets-in-cubeviz-profile branch from c391e54 to c57bc30 Compare June 17, 2025 13:40
Copy link
Copy Markdown
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

Test failures are from our ongoing remote data problems, unrelated to this PR. This is a drastic improvement, as you showed elsewhere I see performance scaling much more linearly with number of subsets now with this combined with the other improvements. Thanks!

Copy link
Copy Markdown
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.

Looks good, thank you!

@kecnry kecnry merged commit 45d3d98 into spacetelescope:main Jun 17, 2025
16 of 20 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/jdaviz that referenced this pull request Jun 17, 2025
kecnry pushed a commit that referenced this pull request Jun 17, 2025
)

Co-authored-by: Thomas Robitaille <thomas.robitaille@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cubeviz performance Performance related 💤backport-v4.2.x on-merge: backport to v4.2.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants