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

Jump to bottom

consider bitmap/contour_visible in logic for top image layer #2818

Merged
merged 3 commits into from
Apr 19, 2024
Merged

consider bitmap/contour_visible in logic for top image layer #2818

merged 3 commits into from
Apr 19, 2024

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Apr 19, 2024 •
edited
Loading

Description

This pull request considers bitmap and contour_visible switches (instead of only the global layer visibility switch from the data menu) in determining the top image layer (used for mouseover). We could also extend this to consider opacity here or in the future.

Screen.Recording.2024-04-19.at.9.27.49.AM.mov

This PR also consolidates some repeated logic between cubeviz and imviz (where the only exception before was allowing for cube data in the cubeviz case which should be able to safely be moved to the more general case).

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)?

@kecnry kecnry added bug Something isn't working 💤 backport-v3.9.x on-merge: backport to v3.9.x labels Apr 19, 2024
@kecnry kecnry added this to the 3.9.1 milestone Apr 19, 2024
@kecnry kecnry force-pushed the mouseover-top-layer-visible branch from 14c39dd to 4e328c4 Compare April 19, 2024 13:41
kecnry
kecnry commented Apr 19, 2024
View reviewed changes
jdaviz/configs/imviz/helper.py
Comment on lines -402 to +406
return layer_is_2d(layer) and not layer.meta.get(_wcs_only_label, False)
return layer_is_2d_or_3d(layer) and not layer.meta.get(_wcs_only_label, False)
Copy link
Member Author

Choose a reason for hiding this comment

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

Doing a search of the codebase, I don't expect this to have any actual consequences anywhere. For Imviz, there is no ability to load cube data, and for cubeviz whenever we call this we really want to include cubes.

Copy link
Contributor

Choose a reason for hiding this comment

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

But what about in the future when we want to support "flexible viz" and could possibly have 2D and 3D data both loaded and Imviz also loaded?

Copy link
Member Author

Choose a reason for hiding this comment

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

we can rename this method if that would help (suggestions welcomed!)... but for now, all uses of it actually mean something that appears like an image on the screen (so either 2d or 3d image data).

@kecnry kecnry force-pushed the mouseover-top-layer-visible branch from 4e328c4 to 0810d83 Compare April 19, 2024 14:29
@kecnry kecnry marked this pull request as ready for review April 19, 2024 15:08
bmorris3
bmorris3 approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@bmorris3 bmorris3 left a comment

Choose a reason for hiding this comment

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

Great.

jdaviz/configs/default/plugins/viewers.py
Comment on lines +319 to +320
layer_is_not_dq(layer.layer) and
(layer.bitmap_visible or layer.contour_visible))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting here (mostly for myself) that this will conflict with #2817.

rosteen
rosteen approved these changes Apr 19, 2024
View reviewed changes
Copy link
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.

LGTM, confirmed it works in both Imviz and Cubeviz.

@kecnry kecnry merged commit a5983b4 into spacetelescope:main Apr 19, 2024
18 checks passed
@lumberbot-app Lumberbot (App)
Copy link

lumberbot-app bot commented Apr 19, 2024

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v3.9.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 a5983b40d4506f9cad68924fa146854712ea98af
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #2818: consider bitmap/contour_visible in logic for top image layer'
  1. Push to a named branch:
git push YOURFORK v3.9.x:auto-backport-of-pr-2818-on-v3.9.x
  1. Create a PR against branch v3.9.x, I would have named this PR:

"Backport PR #2818 on branch v3.9.x (consider bitmap/contour_visible in logic for top image layer)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

kecnry added a commit to kecnry/jdaviz that referenced this pull request Apr 19, 2024
@kecnry
consider bitmap/contour_visible in logic for top image layer (spacete…
a830988 
…lescope#2818)

* consider bitmap/contour_visible in logic for top image layer
* consolidate cubeviz/imviz logic
@kecnry kecnry deleted the mouseover-top-layer-visible branch April 19, 2024 15:53
@kecnry kecnry mentioned this pull request Apr 19, 2024
Merged
kecnry added a commit that referenced this pull request Apr 19, 2024
@kecnry
consider bitmap/contour_visible in logic for top image layer (#2818) (#…
7676d7d 
…2822)

* consider bitmap/contour_visible in logic for top image layer
* consolidate cubeviz/imviz logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rosteen rosteen rosteen approved these changes

@bmorris3 bmorris3 bmorris3 approved these changes

@javerbukh javerbukh Awaiting requested review from javerbukh javerbukh is a code owner

@pllim pllim Awaiting requested review from pllim pllim is a code owner

@haticekaratay haticekaratay Awaiting requested review from haticekaratay haticekaratay is a code owner

@cshanahan1 cshanahan1 Awaiting requested review from cshanahan1 cshanahan1 is a code owner

@gibsongreen gibsongreen Awaiting requested review from gibsongreen gibsongreen is a code owner

Assignees
No one assigned
Labels
bug Something isn't working cubeviz Ready for final review 💤 backport-v3.9.x on-merge: backport to v3.9.x
Projects
None yet
Milestone
3.9.1
Development

Successfully merging this pull request may close these issues.
4 participants
@kecnry @pllim @bmorris3 @rosteen