Adjust crval in resample tests to get overlap with input

[mcara]

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

[schlafly]

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]

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]

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]

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

[mairanteodoro]

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

[mairanteodoro]

I suggest we drop both crpix and crval.

[mairanteodoro]

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

[mcara]

I do not see which tests are failing due to this change. Yes, removing crpix is a different solution but it also will no longer test a specific branch in the code. I think it is important to get these tests working as is even though the tests themselves do not care how the WCS was "made".

[mairanteodoro]

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

[mcara]

@mairanteodoro Sorry for the delay. Which tests are failing?

[mairanteodoro]

Suggested change

crpix=(0, 0),

[mairanteodoro]

Suggested change

crpix=(0, 0),

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.60%. Comparing base (f538ba0) to head (86310f6).
Report is 2 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (f538ba0) and HEAD (86310f6). Click for more details.

HEAD has 5 uploads less than BASE

Flag	BASE (f538ba0)	HEAD (86310f6)
	6	1

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1567       +/-   ##
===========================================
- Coverage   77.94%   28.60%   -49.35%     
===========================================
  Files         115      112        -3     
  Lines        7622     7576       -46     
===========================================
- Hits         5941     2167     -3774     
- Misses       1681     5409     +3728     

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

[schlafly]

Merging as this PR looks good, and the test failures are related to the ongoing unit-removal from romancal reference files.