Skip to content

RCAL-1380: Populate SFD-estimated extinction in catalogs.#2313

Open
mairanteodoro wants to merge 30 commits into
spacetelescope:mainfrom
mairanteodoro:RCAL-1380
Open

RCAL-1380: Populate SFD-estimated extinction in catalogs.#2313
mairanteodoro wants to merge 30 commits into
spacetelescope:mainfrom
mairanteodoro:RCAL-1380

Conversation

@mairanteodoro
Copy link
Copy Markdown
Collaborator

@mairanteodoro mairanteodoro commented May 7, 2026

Resolves RCAL-1380

This PR adds support for populating a new dust_ebv extinction column in Roman source and multiband catalogs using SFD dust maps resolved through CRDS. It updates the catalog schema and source-catalog generation logic, adds tests and regression checks for the new field.

Note

This adjusts CI/tox setup to use temporary git-based dependency installs needed for the change. We need to revert it before merging this PR. Done (unit tests will fail until the RAD PR is merged).

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

mairanteodoro and others added 15 commits May 4, 2026 10:44
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…cified and allow usage of RSDP-specific authentication (spacetelescope#2273)

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

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@mairanteodoro mairanteodoro changed the title Rcal 1380 RCAL-1380: Populate SFD-estimated extinction in catalogs. May 7, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

❌ Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.30%. Comparing base (947f9d1) to head (2b07dbc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
romancal/source_catalog/_source_catalog.py 98.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2313      +/-   ##
==========================================
- Coverage   80.55%   77.30%   -3.26%     
==========================================
  Files         155      155              
  Lines        9398     9451      +53     
==========================================
- Hits         7571     7306     -265     
- Misses       1827     2145     +318     

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

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.

Sorry, I worry I made the dependencies more complicated. Have we changed the procedure there? My memory is that normally we update pyproject.toml to point to your specific rad branch that has the needed updates. I don't understand the ci-gitdeps stuff. Alternatively run regtests overriding the rad dependency to point to your branch and point to the successful regtest run.

Comment thread romancal/regtest/test_multiband_catalog.py Outdated
@mairanteodoro
Copy link
Copy Markdown
Collaborator Author

mairanteodoro commented May 8, 2026

Sorry, I worry I made the dependencies more complicated. Have we changed the procedure there? My memory is that normally we update pyproject.toml to point to your specific rad branch that has the needed updates.

That's usually the case for direct dependencies. However, RAD is not a direct dependency of romancal and editing pyproject.toml won't honor the required version for RAD set in that file (it will be installed via RDM).

@zacharyburnett and I had a quick chat about this issue and he will draft a PR to try and solve this issue of RAD and RDM depending on each other while being two separate packages. For now, patching tox.ini/tests.yml is the temporary solution that I was able to come up with. At least it shows that the guard won't be needed when the RAD changes are in.

Alternatively run regtests overriding the rad dependency to point to your branch and point to the successful regtest run.

The regtests are fine because we can provide additional parameters that will override the requirements:
https://github.com/spacetelescope/RegressionTests/actions/runs/25570563604

In that regtest run, the only failure is for the test_multiband_catalog: romancal.regtest.test_multiband_catalog, and that is because we need to update the truth file later.

@mairanteodoro mairanteodoro requested a review from schlafly May 8, 2026 20:24
@mairanteodoro mairanteodoro added this to the 26Q3_B22 milestone May 8, 2026
@mairanteodoro mairanteodoro marked this pull request as ready for review May 8, 2026 20:39
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.

Looks good to me. One minor comment inline. Thanks Mairan!

Comment thread romancal/regtest/test_source_catalog.py Outdated
@schlafly
Copy link
Copy Markdown
Collaborator

schlafly commented May 11, 2026

The oldestdeps python 3.11 tests were failing with NoneType in various source catalog steps. We probably need a rad version requirement to do this; we're saying that's okay here since it should pass in the regtests with the pyproject / roman_datamodels pointing to the right rad version.

@zacharyburnett
Copy link
Copy Markdown
Collaborator

@zacharyburnett and I had a quick chat about this issue and he will draft a PR to try and solve this issue of RAD and RDM depending on each other while being two separate packages. For now, patching tox.ini/tests.yml is the temporary solution that I was able to come up with. At least it shows that the guard won't be needed when the RAD changes are in.

I drafted spacetelescope/roman_datamodels#676 to see what making a monorepo of rad and roman_datamodels would look like, but from talking with @braingram and @WilliamJamieson I don't think that's a viable solution either unfortunately. For now we'll just have to work with this dependency chain as-is

mairanteodoro and others added 2 commits May 11, 2026 13:02
@mairanteodoro mairanteodoro requested a review from schlafly May 11, 2026 17:25
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.

Thanks, approved. I'll let you back out the pyproject toml changes and merge when the corresponding rad PR is in. We'll also need to okify the differences; I'll let you handle that when you merge?

@mairanteodoro
Copy link
Copy Markdown
Collaborator Author

We'll also need to okify the differences; I'll let you handle that when you merge?

Sounds good.

@braingram
Copy link
Copy Markdown
Collaborator

@zacharyburnett and I had a quick chat about this issue and he will draft a PR to try and solve this issue of RAD and RDM depending on each other while being two separate packages. For now, patching tox.ini/tests.yml is the temporary solution that I was able to come up with. At least it shows that the guard won't be needed when the RAD changes are in.

I drafted spacetelescope/roman_datamodels#676 to see what making a monorepo of rad and roman_datamodels would look like, but from talking with @braingram and @WilliamJamieson I don't think that's a viable solution either unfortunately. For now we'll just have to work with this dependency chain as-is

The unit tests are failing with a TypeError because:

catalog[column].info.description = definition["description"]

is attempting to get the "description" where definition is None. Merging spacetelescope/rad#870 won't fix this as:

We will need:

  • this PR to be updated to point to rdm main
  • a rdm PR setting the required rad version to rad main

@mairanteodoro would you update the rdm requirement in this PR? I'll open a rdm PR setting the rad requirement to main.

@mairanteodoro
Copy link
Copy Markdown
Collaborator Author

Merging spacetelescope/rad#870 won't fix this as:

We will need:

  • this PR to be updated to point to rdm main
  • a rdm PR setting the required rad version to rad main

@mairanteodoro would you update the rdm requirement in this PR? I'll open a rdm PR setting the rad requirement to main.

Yep, I can do that. I assumed that we would have a released version soon, so I didn't think of changing the requirements.

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.

7 participants