RCAL-1342: Source Injection Nan Pixel Support by PaulHuwe · Pull Request #2368 · spacetelescope/romancal

PaulHuwe · 2026-06-27T22:01:27Z

Closes #2022

This PR adds support for injecting sources onto NaN pixels. Previously, if an object was to be injected at the location of a NaN pixel, it would be skipped. This PR will inject the source at a location with a single NaN at the grid point, using an interpolated pixel value for injection. If a source is to be injected in a NaN region, it is skipped.

NOTES:

The injection is coded in such a way that it can be readily expanded to a region of NaN pixels that are some fraction of the source size, if desired.
This PR adds some code duplication in the test file, which was needed to avoid changing other tests. I want to fully clean up this test file in a separate PR.

Tasks

for more information, see https://pre-commit.ci

codecov · 2026-06-27T22:18:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.54%. Comparing base (740761f) to head (9ae612e).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2368      +/-   ##
==========================================
+ Coverage   80.87%   81.54%   +0.67%     
==========================================
  Files         159      157       -2     
  Lines        9531     9406     -125     
==========================================
- Hits         7708     7670      -38     
+ Misses       1823     1736      -87

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

schlafly

Thanks. I identified some of the same code duplication in the testing that you did. I agree that it makes sense to clean this up; I don't think we want this PR to introduce more duplication in preparation for a later PR that removes it?

I left a few other in line comments about the tests.

More importantly, I'm not sure I really get what the purpose of the new block in _multiband_catalog is. If the PR were only the deletion of 302-306 in _injection, what would go wrong?

schlafly · 2026-06-30T13:17:35Z

+    # Ensure all objects matched
+    assert len(matched_grid) == len(results["NoNan"].source_injection_catalog)
+
+    # Test that all matched objects have similar magnitudes


Does this test depend on the fact that the NaNs are far from the sources? Broadly I think it's fine if the magnitudes are wrong if you NaN out the center of a source; I don't think we want this test to depend on how the fake sources are located relative to the NaNs.

The NaNs are only at the center of the injected sources.

Thanks. What is the typical dispersion here? If I understand right, this is matching the recovered magnitude of the source on a partially NaNed image to the recovered magnitude of the same source, on an image without NaNs. Conceptually I expect the recovered magnitude to depend on whether the the image has NaNs. For stars, I expect it to depend a lot because the stars are small and NaNing their centers makes a big impact on the flux estimate you would get. For galaxies, it depends on how big the galaxy is and things are less clear to me. 2 mmag is a very tight tolerance and I worry that this is a sign that something is not working right. What am I missing?

…spacetelescope#2360) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

…etelescope#2352) Co-authored-by: Eddie Schlafly <eschlafly@gmail.com>

Co-authored-by: William Jamieson <william+github@jamieson.phd>

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

…telescope#2367) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

…escope#2324) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Eddie Schlafly <eschlafly@gmail.com>

)

…y, with a checklist for the procedure (spacetelescope#2374)

…pacetelescope#2355) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

for more information, see https://pre-commit.ci

PaulHuwe · 2026-07-10T01:39:44Z

Regression tests pass: https://github.com/spacetelescope/RegressionTests/actions/runs/29050165165

schlafly · 2026-07-10T14:26:10Z

+
+            # Temporarily set NaNs to zero for injections
+            si_model.data[y_pos_idx[nanmask], x_pos_idx[nanmask]] = 0
+            si_model.var_poisson[y_pos_idx[nanmask], x_pos_idx[nanmask]] = 0


Per our discussion Wednesday, the ratio here is used to determine the exposure time, so we should fill with nanmedian or something rather than just zeros (I don't know what exposure time 0/0 would correspond to).

When data and var_poisson is zero, exposure time is 0/1 - the method changes any 0 var_poisson to 1. I was going for the least bespoke change, but I am happy to switch to nanmedian.
https://github.com/spacetelescope/romanisim/blob/main/romanisim/l3.py#L172C1-L177C27

Let's fill with the nanmedians of the whole image so that these sources have sane exposure times.

schlafly · 2026-07-10T14:31:28Z

+    # Ensure all objects matched
+    assert len(matched_grid) == len(results["NoNan"].source_injection_catalog)
+
+    # Test that all matched objects have similar magnitudes


Thanks. What is the typical dispersion here? If I understand right, this is matching the recovered magnitude of the source on a partially NaNed image to the recovered magnitude of the same source, on an image without NaNs. Conceptually I expect the recovered magnitude to depend on whether the the image has NaNs. For stars, I expect it to depend a lot because the stars are small and NaNing their centers makes a big impact on the flux estimate you would get. For galaxies, it depends on how big the galaxy is and things are less clear to me. 2 mmag is a very tight tolerance and I worry that this is a sign that something is not working right. What am I missing?

schlafly · 2026-07-10T14:35:26Z

+    )
+    assert np.allclose(
+        matched_block["kron_f184_abmag"], matched_si_bl["kron_f184_abmag"], rtol=3e-2
+    )


Similarly to the comment above... this is for the case where large chunks of the image are NaN. So we expect some sources to not be recovered at all. I guess in this section there's no assertion that the lengths are identical, reflecting that. But the requirement that the kron mags match at 0.03 mag means that no sources are allowed to be injected near the edge?

Source are injected near the edge, but are not detected.

Injected sources for NaN Block:

Zoom of the center NaN Block corner:

Your suspicions are well founded - during PR revisions, the NANPOINTS drifted. I am rewriting that section (and tests) to be less brittle.

Co-authored-by: Eddie Schlafly <eschlafly@gmail.com>

PaulHuwe added 4 commits June 10, 2026 16:52

Save

ab1ee7c

Save

66e9632

PR Save

02d65b8

Added library nan value tests

ce9841b

PaulHuwe requested a review from schlafly June 27, 2026 22:01

github-actions Bot added the testing label Jun 27, 2026

[pre-commit.ci] auto fixes from pre-commit.com hooks

9b47e99

for more information, see https://pre-commit.ci

stscijgbot-rstdms mentioned this pull request Jun 27, 2026

Revise Source Injection NaN Handling #2022

Open

schlafly reviewed Jun 30, 2026

View reviewed changes

braingram and others added 16 commits July 9, 2026 13:36

make test compatible with pytest 9.1

f2fec29

Fix read noise sqrt(2) factor in likely ramp fitting; flat field dumo (…

4d74087

…spacetelescope#2360) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

allow astropy 8 (spacetelescope#2364)

72787bc

Fix save_results handling (remove custom source catalog saving) (spac…

92e4ece

…etelescope#2352) Co-authored-by: Eddie Schlafly <eschlafly@gmail.com>

ignore crds context for regtest differences (spacetelescope#2363)

f873460

Co-authored-by: William Jamieson <william+github@jamieson.phd>

[pre-commit.ci] pre-commit autoupdate (spacetelescope#2350)

0fb240f

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

relax tolerances for tvac test (spacetelescope#2365)

bba917a

RCAL-1412: Implement ability to choose local pysiaf xml folder (space…

62fb374

…telescope#2367) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Update resampled output image's background level and status (spacetel…

ba24160

…escope#2324) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Eddie Schlafly <eschlafly@gmail.com>

dynamically test with all supported Python versions (spacetelescope#2256

fed06a3

)

[SCSB-270] add issue template for requesting a pipeline build deliver…

c09f818

…y, with a checklist for the procedure (spacetelescope#2374)

switch roman_datamodels dependency back to main (spacetelescope#2380)

da6ad1c

remove ready_for_review trigger (spacetelescope#2381)

8e2a18d

Store indices of detected jumps in images (spacetelescope#2376)

4e4647c

Rcal-1395 Make association public/private and remove unused modules (s…

6b81fc6

…pacetelescope#2355) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Rebase

07de5e5

PaulHuwe force-pushed the RCAL-1342_SINaN branch from 9b47e99 to 61f476f Compare July 9, 2026 18:21

Added changelog

c009bd2

PaulHuwe force-pushed the RCAL-1342_SINaN branch from 73ceeaa to c009bd2 Compare July 9, 2026 19:01

github-actions Bot added the documentation Improvements or additions to documentation label Jul 9, 2026

github-actions Bot added ramp_fitting pipeline stpipe dependencies Pull requests that update a dependency file regression_testing associations Wide Field Instrument (WFI) automation flatfield labels Jul 9, 2026

Addressed ruff errors

abf8280

PaulHuwe force-pushed the RCAL-1342_SINaN branch from 1a56d26 to abf8280 Compare July 9, 2026 20:09

PaulHuwe and others added 2 commits July 9, 2026 16:11

Merge branch 'main' into RCAL-1342_SINaN

0b1293b

[pre-commit.ci] auto fixes from pre-commit.com hooks

0f7423f

for more information, see https://pre-commit.ci

PaulHuwe marked this pull request as ready for review July 10, 2026 01:40

PaulHuwe requested review from a team, larrybradley, mairanteodoro and stscieisenhamer as code owners July 10, 2026 01:40

PaulHuwe requested a review from schlafly July 10, 2026 01:40

schlafly reviewed Jul 10, 2026

View reviewed changes

Update romancal/multiband_catalog/tests/test_multiband_catalog.py

9ae612e

Co-authored-by: Eddie Schlafly <eschlafly@gmail.com>

Uh oh!

Conversation

PaulHuwe commented Jun 27, 2026

Tasks

Uh oh!

codecov Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

schlafly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PaulHuwe commented Jul 10, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov Bot commented Jun 27, 2026 •

edited

Loading