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

TST: Increase Imviz rotation test coverage #2712

Merged
merged 8 commits into from
Feb 27, 2024
Merged

TST: Increase Imviz rotation test coverage #2712

merged 8 commits into from
Feb 27, 2024

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Feb 16, 2024 •
edited
Loading

Description

This pull request is a follow-up of #2179 to increase test coverage and prevent future regressions (within reason).

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

@pllim pllim added testing no-changelog-entry-needed changelog bot directive Affects-dev changelog bot directive labels Feb 16, 2024
@pllim pllim added this to the 3.9 milestone Feb 16, 2024
@github-actions github-actions bot added the imviz label Feb 16, 2024
@pllim pllim force-pushed the this-is-not-the-greatest-test-in-the-world-no-it-is-just-a-tribute branch 2 times, most recently from e57e4b8 to 7d6369f Compare February 21, 2024 00:10
pllim
pllim commented Feb 21, 2024
View reviewed changes
jdaviz/configs/imviz/wcs_utils.py
@@ -564,7 +564,7 @@ def compute_scale(wcs, fiducial, disp_axis, pscale_ratio=1):
"""
spectral = 'SPECTRAL' in wcs.output_frame.axes_type

if spectral and disp_axis is None:
if spectral and disp_axis is None: # pragma: no cover
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: This was copied from jwst so if spectral is probably important there but we do not care here.

@pllim pllim force-pushed the this-is-not-the-greatest-test-in-the-world-no-it-is-just-a-tribute branch from 7d6369f to 943b26d Compare February 21, 2024 22:13
@codecov Codecov
Copy link

codecov bot commented Feb 21, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.66%. Comparing base (a50aca4) to head (63b460b).
Report is 6 commits behind head on main.

❗ Current head 63b460b differs from pull request most recent head 0336005. Consider uploading reports for the commit 0336005 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2712      +/-   ##
==========================================
+ Coverage   88.07%   88.66%   +0.59%     
==========================================
  Files         108      108              
  Lines       15883    15886       +3     
==========================================
+ Hits        13989    14086      +97     
+ Misses       1894     1800      -94

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

@pllim pllim marked this pull request as ready for review February 22, 2024 03:03
kecnry
kecnry reviewed Feb 22, 2024
View reviewed changes
jdaviz/configs/imviz/tests/test_orientation.py
Comment on lines +49 to +51
# Blink to second image, if we have to.
if self.viewer.top_visible_data_label != "has_wcs_2[SCI,1]":
self.viewer.blink_once()
Copy link
Member

Choose a reason for hiding this comment

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

does this ever happen (ie is it random)? Maybe just put an assert statement here to ensure that the top layer is as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I have multiple test methods, I want to guard against chances when they are run out of order (could be possible if xdist is involved).

jdaviz/configs/imviz/tests/test_orientation.py Outdated Show resolved Hide resolved
@pllim pllim mentioned this pull request Feb 22, 2024
Closed
@pllim pllim force-pushed the this-is-not-the-greatest-test-in-the-world-no-it-is-just-a-tribute branch from 2e662ab to 0f96140 Compare February 22, 2024 23:17
@pllim pllim force-pushed the this-is-not-the-greatest-test-in-the-world-no-it-is-just-a-tribute branch from 0f96140 to 3d5e6b1 Compare February 26, 2024 14:38
@pllim pllim force-pushed the this-is-not-the-greatest-test-in-the-world-no-it-is-just-a-tribute branch from fbf8d7f to 59286c0 Compare February 26, 2024 14:45
@pllim
Copy link
Contributor Author

pllim commented Feb 26, 2024

From Brett:

I don’t have any idea. My only lead would be to check what the refdata are throughout this test, since that’s the place

(

jdaviz/jdaviz/configs/default/plugins/markers/markers.py

Line 133 in 3adc659

new_wcs = viewer.state.reference_data.coords

where things are most likely to go wrong in the markers updates during link type change.

pllim and others added 5 commits February 27, 2024 12:02
@pllim @kecnry
TST: Use as_active context manager
25c089c 
Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
@pllim
Confirmed bug is follow-up issue.
26a704a 
Add regression test for JDAT-3958
but cannot reproduce the failure.
@pllim @rosteen
TST: Reproduce the reparenting failure.
75a720f 
Co-authored-by:  Ricky O'Steen <39831871+rosteen@users.noreply.github.com>
@pllim
Mark test as xfail
1a2be30 
and turn xpass into failure.
@pllim
Update test with expected answers
ccbab4e 
but this would fail the test, so catch the failures for now
@pllim
Copy link
Contributor Author

pllim commented Feb 27, 2024

@kecnry , the fake WCS just does not roundtrip, I don't know what to do about this. The mark does appear "correct" visually when linked by WCS:

Screenshot 2024-02-27 123840

If you extend the test case, you would see this:

>>> sky = wcs_1.pixel_to_world(0, 0)  # First loaded data
>>> sky
<SkyCoord (ICRS): (ra, dec) in deg
    (337.5202808, -20.83333306)>
>>> viewer.state.reference_data.coords.world_to_pixel(sky)  # Default orientation
[-0.25000000137056677, -0.24999999861013933]

How do you want to proceed?

  1. Accept the test as-is.
  2. A better way to check for new mark positions. Since you wrote the plugin, if there is a better way, you would know.
  3. Mark this test as xfail and open a bug ticket of some sort.

Please advise. Thanks!

@pllim pllim force-pushed the this-is-not-the-greatest-test-in-the-world-no-it-is-just-a-tribute branch from 5443c5a to 63b460b Compare February 27, 2024 17:45
@kecnry
Copy link
Member

kecnry commented Feb 27, 2024

Ok, since it's already in main, I'd vote for getting the test in marked to fail and open a bug ticket.

@pllim
Copy link
Contributor Author

pllim commented Feb 27, 2024

OK https://jira.stsci.edu/browse/JDAT-4256

@pllim pllim added the trivial Only needs one approval instead of two label Feb 27, 2024
@pllim pllim enabled auto-merge February 27, 2024 18:12
@pllim
Copy link
Contributor Author

pllim commented Feb 27, 2024

I think one approval is enough. There is no functionality change. Thanks!

@pllim pllim merged commit 770573e into spacetelescope:main Feb 27, 2024
17 of 18 checks passed
@pllim pllim deleted the this-is-not-the-greatest-test-in-the-world-no-it-is-just-a-tribute branch February 27, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kecnry kecnry kecnry approved these changes

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

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

@bmorris3 bmorris3 Awaiting requested review from bmorris3 bmorris3 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
Affects-dev changelog bot directive imviz no-changelog-entry-needed changelog bot directive testing trivial Only needs one approval instead of two
Projects
None yet
Milestone
3.9
Development

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