Skip to content

Implement method to populate meta.statistics attribute for datamodels.#2248

Open
mairanteodoro wants to merge 11 commits intospacetelescope:mainfrom
mairanteodoro:RCAL-999
Open

Implement method to populate meta.statistics attribute for datamodels.#2248
mairanteodoro wants to merge 11 commits intospacetelescope:mainfrom
mairanteodoro:RCAL-999

Conversation

@mairanteodoro
Copy link
Copy Markdown
Collaborator

@mairanteodoro mairanteodoro commented Mar 30, 2026

Resolves RCAL-999

This PR implements a new romancal.lib.statistics.populate_statistics() helper and wires it into RomanStep.finalize_result() so that any Roman step that produces an ImageModel or MosaicModel output will have meta.statistics populated automatically.

Regression tests

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 changelog readme for instructions)
      • if your change breaks existing functionality, also add a changes/<PR#>.breaking.rst news fragment
    • 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

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.63%. Comparing base (683c428) to head (cfd7106).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2248      +/-   ##
==========================================
+ Coverage   80.58%   80.63%   +0.05%     
==========================================
  Files         155      156       +1     
  Lines        9341     9368      +27     
==========================================
+ Hits         7527     7554      +27     
  Misses       1814     1814              

☔ View full report in Codecov by Sentry.
📢 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.

@mairanteodoro mairanteodoro marked this pull request as ready for review March 30, 2026 21:18
@mairanteodoro mairanteodoro requested review from a team and ddavis-stsci as code owners March 30, 2026 21:18
Copy link
Copy Markdown
Collaborator

@ddavis-stsci ddavis-stsci left a comment

Choose a reason for hiding this comment

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

LGTM

@braingram
Copy link
Copy Markdown
Collaborator

Thanks for putting this together. Now that statistics is populated would you remove the "junk" value setting in resample:

# TODO statistics are unknown
model.meta.statistics.image_median = np.nan
model.meta.statistics.image_rms = np.nan
model.meta.statistics.good_pixel_fraction = np.nan

@mairanteodoro mairanteodoro changed the title Implement method to populate meta.statistics attribute for L2 models. Implement method to populate meta.statistics attribute for datamodels. Mar 31, 2026
@mairanteodoro mairanteodoro requested a review from braingram March 31, 2026 21:15
Copy link
Copy Markdown
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

Thanks! Code updates look good to me. Let's wait for @schlafly to weight in on the differences and to clarify what should be done for L3 files. Specifically, should they have zodiacal_light populated?

Copy link
Copy Markdown
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

This looks like a good implementation to me; I would not have thought of doing it through finalize_result, but it's nice that that means that all intermediate products get flags too.

I have a suggestion below for only computing the medians and rmses from the good pixels, though I don't expect that to make a meaningful difference in practice.

Let's not populate zodiacal_light for L3 files; it's not in the schema and it doesn't make a lot of sense.

@schlafly
Copy link
Copy Markdown
Collaborator

schlafly commented Apr 8, 2026

It looks like my suggestion fails because err does not exist. It's required, though, so I'm a bit confused; presumably some test code hadn't needed it before and now is failing?

@mairanteodoro
Copy link
Copy Markdown
Collaborator Author

Yes, I'm updating the code and unit tests to account for that.

pre-commit-ci bot and others added 5 commits April 8, 2026 13:52
It only includes pixels that pass three tests:
- DQ flag is clean (DO_NOT_USE is not set);
- error/uncertainty is positive (model.err > 0);
- the data is finite (NaN and Inf are removed).

Co-authored-by: Eddie Schlafly <eschlafly@gmail.com>
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