-
Notifications
You must be signed in to change notification settings - Fork 74
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
BUG: Fix click to center on multiple visible layers when linked by WCS #2750
Conversation
visible layers when linked by WCS.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2750 +/- ##
==========================================
+ Coverage 88.63% 88.68% +0.05%
==========================================
Files 108 108
Lines 16210 16214 +4
==========================================
+ Hits 14367 14379 +12
+ Misses 1843 1835 -8 ☔ View full report in Codecov by Sentry. |
Looks like this fixed the issue, I'll give a real review/approval once it's ready. |
This comment was marked as resolved.
This comment was marked as resolved.
and I had proven that the test fails when linked by WCS without this patch
cur_cen = v._get_center_skycoord() | ||
v.center_on(coo) | ||
real_cen = v._get_center_skycoord() | ||
assert_allclose(cur_cen.ra.deg, real_cen.ra.deg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this patch, it would fail with this error here:
E AssertionError:
E Not equal to tolerance rtol=1e-07, atol=0
E
E Mismatched elements: 1 / 1 (100%)
E Max absolute difference: 0.00015008
E Max relative difference: 7.58381226e-07
E x: array(197.892778)
E y: array(197.892628)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me - thanks for the quick fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Fixes #2749
Change log entry
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, maintainershould 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.
trivial
label.