Skip to content

RCAL-1074: Implement integration test#23

Merged
mairanteodoro merged 31 commits intospacetelescope:mainfrom
mairanteodoro:RCAL-1074
Jul 7, 2025
Merged

RCAL-1074: Implement integration test#23
mairanteodoro merged 31 commits intospacetelescope:mainfrom
mairanteodoro:RCAL-1074

Conversation

@mairanteodoro
Copy link
Collaborator

@mairanteodoro mairanteodoro commented Jun 2, 2025

Resolves RCAL-1074

This PR addresses the implementation of an end-to-end test for roman_photoz and some improvements.

To run roman_photoz:

  • install it locally:
pip install -e .
  • run it on the command line:
roman-photoz \
--input-path ./roman_photoz/data \
--input-filename r00001_p_v01001001001001_270p65x49y70_f158_mbcat_cat.parquet \
--output-path ./roman_photoz/data \
--output-filename result.parquet

Note that r00001_p_v01001001001001_270p65x49y70_f158_mbcat_cat.parquet is located in the roman_photoz/data folder and is a simple template catalog containing observations in only one filter (F158).

Moving forward, the name of the column to be used for fitting can now be provided as a "template," which means the user can provide the name of the columns for flux/mag and corresponding error as a string with a placeholder for the Roman filter ID as fNNN. For example, "psf_{}_flux" and "psf_{}_flux_err" are both valid templates, which means that roman_photoz will look for columns in the catalog that match the provided template applied for each filter ID.

The code is compliant with the latest catalog format but does not actively use the r00001_p_v01001001001001_r274dp63x31y81_f158_mbcat_cat.parquet file for the integration test as it is not a "true" multiband catalog (it only contains data for one filter). Instead, we're using a simulated multiband catalog that was produced by roman_photoz.create_simulated_catalog.py.

All the unit tests and integration test are passing.

Once this PR is merged, the docs will be updated in a following PR.

@mairanteodoro mairanteodoro requested a review from schlafly June 2, 2025 18:07
Copy link
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.

Some comments inline. This looks good. Structurally I'm most concerned about duplicating confest.py and regtest.py; is code reuse possible there instead?

@mairanteodoro mairanteodoro requested a review from schlafly June 30, 2025 14:26
Copy link
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.

Let's find a time to chat about this a little.

A few thoughts in addition to the stuff inline:

  • Is the context ever being "correctly" set anywhere? i.e., if a file only contains 4 filters, does the context mark that only those four filters were good?
  • I couldn't figure out where the "name changing" with the magnitude columns was happening, from the given fit_colnames. Maybe here? , and there was a rail update or you realized that you could avoid the extra reformatting done in format_data?

"Z_RANGE": "0.,99.99",
"Z_STEP": "0.1,0.,7.",
"Z_RANGE": "0.01,99.99",
"Z_STEP": "0.001,0.001,1.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you say a few words about the changes here? ebv starting at 0 vs. 0.01 sounds fine but I don't really understand the need for the change. CAT_MAG = AB sounds like a strict improvement. I don't understand the EMLINES change (but mostly because I don't understand the EMLINES feature in the first place; what is motivating this change, or what is this change doing?). It looks like the extinction law change causes all 4 extinction curves to be used on "models 1-31", but I'm not immediately sure if those are all of the models or what; is this a default configuration change from upstream? For the z range, maybe avoiding z = 0 makes sense. But only going out to 1.0 will cause problems. Is the finer binning necessary?

@@ -0,0 +1,188 @@
# Regression Test Framework
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just refer to the romancal docs, except for the differences at the end?

2. File caching with control functions
3. Better handling of binary vs text file differences
4. More robust pytest configuration
5. Comprehensive documentation and examples
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be putting these changes in romancal instead?

@mairanteodoro mairanteodoro requested a review from schlafly July 2, 2025 18:31
Copy link
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 Mairan. Can you remove or point to the romancal docs for the regtest readme, instead of including new documentation there? Since the code is the same I don't think we should have duplicated docs. Can you also change the default flux type to the segment flux instead of the psf flux? Otherwise looks good.

@mairanteodoro
Copy link
Collaborator Author

Can you remove or point to the romancal docs for the regtest readme, instead of including new documentation there?

Done.

Can you also change the default flux type to the segment flux instead of the psf flux?

Done.

@mairanteodoro mairanteodoro requested a review from schlafly July 7, 2025 14:13
Copy link
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, looks good!

@mairanteodoro mairanteodoro merged commit 4367954 into spacetelescope:main Jul 7, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants