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

set bounding box to None to avoid layer cropping #1908

Merged
merged 15 commits into from
Dec 20, 2022

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Dec 8, 2022

Description

This pull request fixes WCS without fast-approximation resulting in layer cropping for images with GWCS by disabling the bounding box. This also allows the mouseover coordinates to extrapolate across the entire viewer based on the G/WCS of the reference image.

Fixes #1905, fixes #1899, fixes #1509

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

@kecnry kecnry added imviz 💤backport-v3.1.x on-merge: backport to v3.1.x labels Dec 8, 2022
@kecnry kecnry added this to the 3.1.2 milestone Dec 8, 2022
@kecnry kecnry force-pushed the fix-gwcs-bounding-box branch 2 times, most recently from 9bd5e14 to aae0bd7 Compare December 8, 2022 19:54
kecnry added a commit to kecnry/jdaviz that referenced this pull request Dec 8, 2022
@kecnry kecnry marked this pull request as ready for review December 8, 2022 20:21
kecnry added a commit to kecnry/jdaviz that referenced this pull request Dec 8, 2022
kecnry added a commit to kecnry/jdaviz that referenced this pull request Dec 8, 2022
CHANGES.rst Outdated Show resolved Hide resolved
@mcara
Copy link
Member

mcara commented Dec 13, 2022

@pllim just to remind you: astropy/astropy#9073 Also, see the related/linked issue in gwcs. If you can handle astropy.wcs.WCS separately from gwcs.wcs.WCS then you could simply set with_bounding_box=False.

@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Base: 91.72% // Head: 91.79% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (0247f8c) compared to base (d2282d2).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1908      +/-   ##
==========================================
+ Coverage   91.72%   91.79%   +0.07%     
==========================================
  Files         140      140              
  Lines       14824    14951     +127     
==========================================
+ Hits        13597    13724     +127     
  Misses       1227     1227              
Impacted Files Coverage Δ
...igs/default/plugins/subset_plugin/subset_plugin.py 98.54% <100.00%> (ø)
...z/configs/imviz/plugins/coords_info/coords_info.py 97.87% <100.00%> (+0.31%) ⬆️
jdaviz/configs/imviz/plugins/parsers.py 100.00% <100.00%> (ø)
jdaviz/configs/imviz/plugins/tools.py 94.07% <100.00%> (ø)
jdaviz/configs/imviz/plugins/viewers.py 88.77% <100.00%> (+0.47%) ⬆️
jdaviz/configs/imviz/tests/test_linking.py 100.00% <100.00%> (ø)
jdaviz/configs/imviz/tests/utils.py 100.00% <100.00%> (ø)
jdaviz/configs/imviz/wcs_utils.py 89.65% <100.00%> (+1.30%) ⬆️
jdaviz/core/region_translators.py 98.98% <100.00%> (ø)

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.

@kecnry
Copy link
Member Author

kecnry commented Dec 13, 2022

@pllim @rosteen - this is what I came up with for the mouseover display, let me know what you think. The text changes to a gray and shows "(est.)" below the "World" entry to indicate the coordinates are less reliable since they are outside the original bounding box.

Note here the "E"/red image is the top-layer showing flux values, whereas the "A"/gray image is the reference data, so value shows when hovering over red, and sky coordinates are estimated when not over gray.

Screen.Recording.2022-12-13.at.4.11.25.PM.mov

@kecnry kecnry force-pushed the fix-gwcs-bounding-box branch 3 times, most recently from 6c2927a to 4f46f3a Compare December 13, 2022 22:41
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Code-wise LGTM but I have suggestions on clarifying this new feature in the doc. Overall, the solution is very clean. Thanks!

CHANGES.rst Outdated Show resolved Hide resolved
docs/imviz/displayimages.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
docs/imviz/displayimages.rst Outdated Show resolved Hide resolved
pllim and others added 4 commits December 19, 2022 10:08
* outside reference data's bounding box makes both pixel and sky coordinates "estimated"
* outside active layer's bounding box makes the pixel coordinates "estimated"
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
@kecnry kecnry marked this pull request as ready for review December 19, 2022 15:47
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Need to step away for lunch break. Just posting this now so I don't lose it if I lose power. I am still looking at this. If you wanna apply now, feel free.

jdaviz/configs/imviz/plugins/parsers.py Outdated Show resolved Hide resolved
docs/imviz/displayimages.rst Outdated Show resolved Hide resolved
jdaviz/configs/imviz/plugins/viewers.py Outdated Show resolved Hide resolved
jdaviz/configs/imviz/plugins/viewers.py Show resolved Hide resolved
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

I was pretty thorough with this and I am happy on its current state. Though I am also a co-author now, so take my approval however you want. 😬

Thanks for tackling this, Kyle!

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.

Seems to work in my testing, and the test coverage looks good. I left one question about a class name but it's not a blocker/crucial to merge.

@kecnry kecnry merged commit 35cd433 into spacetelescope:main Dec 20, 2022
@lumberbot-app

This comment was marked as resolved.

kecnry added a commit to kecnry/jdaviz that referenced this pull request Dec 20, 2022
* set bounding box to None to avoid layer cropping
* mouseover display to warn when outside original bounding box

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
kecnry added a commit to kecnry/jdaviz that referenced this pull request Dec 20, 2022
…yer cropping

* set bounding box to None to avoid layer cropping
* mouseover display to warn when outside original bounding box

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
@kecnry kecnry deleted the fix-gwcs-bounding-box branch December 20, 2022 18:30
pllim added a commit to kecnry/jdaviz that referenced this pull request Dec 20, 2022
…yer cropping

* set bounding box to None to avoid layer cropping
* mouseover display to warn when outside original bounding box

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
pllim added a commit to pllim/jdaviz that referenced this pull request Dec 20, 2022
kecnry added a commit that referenced this pull request Dec 20, 2022
…pping (#1942)

* Backport PR #1908: set bounding box to None to avoid layer cropping

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants