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

Ensure ImViz works well with arbitrary BaseCartesianData instances #1904

Closed
wants to merge 1 commit into from
Closed

Ensure ImViz works well with arbitrary BaseCartesianData instances #1904

wants to merge 1 commit into from

Conversation

astrofrog
Copy link
Collaborator

@astrofrog astrofrog commented Dec 8, 2022 •
edited by pllim
Loading

Description

This is part of work towards implementing image rotation via on-the-fly reprojection. The main changes here are to support the most general glue data class, BaseCartesianData of which Data is one instance.

Depends on:

Needs to make sure this also accounts for:

  • set bounding box to None to avoid layer cropping #1908
  • Performance
  • Not breaking Imviz plugins
  • When angle is zero, WCS should completely roundtrip even if this means we keep a copy of the original WCS for this purpose
  • Different combos: FITS WCS or GWCS or no WCS all can be loaded together; pixel vs WCS linking...Wwhat if you rotate reference image? What if you rotate not the reference image?

🐸

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

@github-actions github-actions bot added the imviz label Dec 8, 2022
astrofrog
astrofrog commented Dec 8, 2022
View reviewed changes
jdaviz/configs/imviz/plugins/viewers.py
self.label_mouseover.icon = self.jdaviz_app.state.layer_icons.get(active_layer.layer.label) # noqa
icon = self.jdaviz_app.state.layer_icons.get(active_layer.layer.label)
if icon is not None:
self.label_mouseover.icon = icon
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The icon doesn't work well for arbitrary BaseCartesianData instances at the moment, this needs to be investigated in glue-core but for now this ensures a graceful fallback if an icon is not found (which we should do anyway).

Copy link
Member

Choose a reason for hiding this comment

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

We should probably set to an empty string or have the icon itself smart enough to handle None in layer_viewer_icon.vue (rather than just leave it at its previous value). I'm not sure why BaseCartesianData would have any problem with the layer icon logic though, layer_icons gets populated in

jdaviz/jdaviz/app.py

Lines 384 to 386 in f36f3b2

if layer_name not in self.state.layer_icons:
self.state.layer_icons = {**self.state.layer_icons,
layer_name: alpha_index(len(self.state.layer_icons))}
which should be called by any AddDataMessage. Is it possible the AddDataMessage isn't being sent from glue for BaseCartesianData?

astrofrog
astrofrog commented Dec 8, 2022
View reviewed changes
jdaviz/configs/imviz/plugins/viewers.py
@@ -126,8 +129,11 @@ def on_mouse_or_key_event(self, data):
and x < image.shape[1] - 0.5 and y < image.shape[0] - 0.5
and hasattr(active_layer, 'attribute')):
attribute = active_layer.attribute
value = image.get_data(attribute)[int(round(y)), int(round(x))]
unit = image.get_component(attribute).units
value = image.get_data(attribute, view=[np.atleast_1d(y), np.atleast_1d(x)])[0]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This ensures that we only ever access the relevant pixel and don't accidentally load all the data then slice it.

astrofrog
astrofrog commented Dec 8, 2022
View reviewed changes
jdaviz/configs/imviz/plugins/viewers.py
if isinstance(image, Data):
unit = image.get_component(attribute).units
else:
unit = ''
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The existence of components with units is only guaranteed for Data subclasses not BaseCartesianData subclasses

@pllim pllim mentioned this pull request Dec 14, 2022
Closed
15 tasks
@pllim
Copy link
Contributor

pllim commented Dec 19, 2022

We also have to make sure this will play nice with #1908

@pllim

This comment was marked as resolved.

Sign in to view
@pllim pllim marked this pull request as draft December 19, 2022 20:52
@pllim
Copy link
Contributor

pllim commented Dec 19, 2022

p.s. Turning this to draft because this is still experimental. Thanks!

@pllim pllim mentioned this pull request Jan 20, 2023
Closed
17 tasks
@pllim pllim mentioned this pull request Jan 30, 2023
Merged
20 tasks
@pllim
Copy link
Contributor

pllim commented Mar 29, 2023

Superseded by #1983

@pllim pllim closed this Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duytnguyendtn duytnguyendtn Awaiting requested review from duytnguyendtn

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

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

@ojustino ojustino Awaiting requested review from ojustino

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

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

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

@haticekaratay haticekaratay Awaiting requested review from haticekaratay haticekaratay will be requested when the pull request is marked ready for review haticekaratay is a code owner

@cshanahan1 cshanahan1 Awaiting requested review from cshanahan1 cshanahan1 will be requested when the pull request is marked ready for review cshanahan1 is a code owner

@gibsongreen gibsongreen Awaiting requested review from gibsongreen gibsongreen will be requested when the pull request is marked ready for review gibsongreen is a code owner

Assignees
No one assigned
Labels
imviz
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@astrofrog @pllim @kecnry