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

Adjust crval in resample tests to get overlap with input #1567

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mcara
Copy link
Member

@mcara mcara commented Dec 19, 2024

This PR adjusts crval value in test_populate_mosaic_basic_single_exposure and test_populate_mosaic_basic_different_observations unit tests. The original value was 10 degrees off due to which input images do not have any overlap with output image created with custom WCS parameters. With new changes in GWCS (enabling bounding box for inverse + a fix in stcal) this would lead to tests failing.

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. 24Q4_B15 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly
news fragment change types...
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • changes/<PR#>.docs.rst
  • changes/<PR#>.stpipe.rst
  • changes/<PR#>.associations.rst
  • changes/<PR#>.scripts.rst
  • changes/<PR#>.mosaic_pipeline.rst
  • changes/<PR#>.patch_match.rst

steps

  • changes/<PR#>.dq_init.rst
  • changes/<PR#>.saturation.rst
  • changes/<PR#>.refpix.rst
  • changes/<PR#>.linearity.rst
  • changes/<PR#>.dark_current.rst
  • changes/<PR#>.jump_detection.rst
  • changes/<PR#>.ramp_fitting.rst
  • changes/<PR#>.assign_wcs.rst
  • changes/<PR#>.flatfield.rst
  • changes/<PR#>.photom.rst
  • changes/<PR#>.flux.rst
  • changes/<PR#>.source_detection.rst
  • changes/<PR#>.tweakreg.rst
  • changes/<PR#>.skymatch.rst
  • changes/<PR#>.outlier_detection.rst
  • changes/<PR#>.resample.rst
  • changes/<PR#>.source_catalog.rst

@mcara mcara requested a review from a team as a code owner December 19, 2024 20:12
@mcara mcara requested review from braingram and mairanteodoro and removed request for a team December 19, 2024 20:12
@mcara mcara requested a review from schlafly December 19, 2024 20:12
@mcara mcara self-assigned this Dec 19, 2024
@schlafly
Copy link
Collaborator

Thanks. Mostly for my reference, the issue is that the wfi_scas were populated at (ra, dec) = (10, 0), but the corresponding crvals were populated at (0, 0), as Mihai says.
https://github.com/spacetelescope/romancal/pull/1567/files#diff-3263dc8226f60646b134491bd47fad24fecde7e8904e07bb43253e695e8c385eL174

@braingram
Copy link
Collaborator

braingram commented Dec 19, 2024

It looks like both of those tests are only looking at the metadata for a blended model. Would they work with the upcoming changes if crval was removed? I don't think that would have any negative impact on the test. It looks like both are only computing the output_wcs to get an array shape to use to make an output model and then calling populate_mosaic_basic. Perhaps the array shape can just be hard coded?

EDIT: I was writing this while Eddie was reviewing. The above changes could be a follow-up PR (if suitable).

@mcara
Copy link
Member Author

mcara commented Dec 19, 2024

It looks like both of those tests are only looking at the metadata for a blended model. Would they work with the upcoming changes if crval was removed? I don't think that would have any negative impact on the test. It looks like both are only computing the output_wcs to get an array shape to use to make an output model and then calling populate_mosaic_basic. Perhaps the array shape can just be hard coded?

EDIT: I was writing this while Eddie was reviewing. The above changes could be a follow-up PR (if suitable).

There are different options:

  1. array shape can be hard-coded;
  2. crpix can be removed;
  3. or have crpix "agree" with "crval" so that there are at least some valid data;
  4. or just accept this PR.

@braingram
Copy link
Collaborator

Thanks @mcara I opened an issue to track the follow up changes #1569

@mairanteodoro
Copy link
Collaborator

Running the changes in this PR against https://github.com/mcara/stcal/tree/fix-bbox-shift-make-wcs still causes the two tests to fail. Removing crpix and leaving crval (regardless of its value) works.

Since the idea behind those tests is to check for the functionality of populate_mosaic_basic(), I suggest that we drop both crpix and crval (they are computed automatically by make_output_wcs() anyways).

Copy link
Collaborator

@mairanteodoro mairanteodoro left a comment

Choose a reason for hiding this comment

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

I suggest we drop both crpix and crval.

@@ -715,7 +715,7 @@ def test_populate_mosaic_basic_single_exposure(exposure_1):
rotation=0,
shape=None,
crpix=(0, 0),
crval=(0, 0),
crval=(10, 0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tests are still failing with crpix=(0,0) + crval=(10,0). It does work when removing crpix alone (regardless of crval's value).

@@ -1014,7 +1014,7 @@ def test_populate_mosaic_basic_different_observations(
rotation=0,
shape=None,
crpix=(0, 0),
crval=(0, 0),
crval=(10, 0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tests are still failing with crpix=(0,0) + crval=(10,0). It does work when removing crpix alone (regardless of crval's value).

@@ -715,7 +715,7 @@ def test_populate_mosaic_basic_single_exposure(exposure_1):
rotation=0,
shape=None,
crpix=(0, 0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
crpix=(0, 0),

@@ -1014,7 +1014,7 @@ def test_populate_mosaic_basic_different_observations(
rotation=0,
shape=None,
crpix=(0, 0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
crpix=(0, 0),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants