spacetelescope / romancal

Python library to process science observations from the Nancy Grace Roman Space Telescope
https://roman-pipeline.readthedocs.io/en/latest/
Other
28 stars 28 forks source link

RCAL-777: allow `Step._datamodels_open` to open association files #1270

Closed braingram closed 2 weeks ago

braingram commented 3 weeks ago

Romancal currently fails if crds pars file fetching is not disabled due to a bug in Step._datamodels_open:

>>> strun roman_mos foo.json
...
TypeError: Open requires a filepath, file-like object, or Roman datamodel

This PR fixes _datamodels_open to open:

allowing crds pars file fetching. Note that these files don't appear to currently exist on roman crds so running the above mosaic pipeline example with this PR will produce the expected log messages (and no longer produce an exception):

2024-06-11 16:01:07,668 - CRDS - ERROR -  Error determining best reference for 'pars-fluxstep'  =   Unknown reference type 'pars-fluxstep'
2024-06-11 16:01:07,669 - CRDS - ERROR -  Error determining best reference for 'pars-skymatchstep'  =   Unknown reference type 'pars-skymatchstep'
2024-06-11 16:01:07,670 - CRDS - ERROR -  Error determining best reference for 'pars-outlierdetectionstep'  =   Unknown reference type 'pars-outlierdetectionstep'
2024-06-11 16:01:07,670 - CRDS - ERROR -  Error determining best reference for 'pars-resamplestep'  =   Unknown reference type 'pars-resamplestep'
2024-06-11 16:01:07,671 - CRDS - ERROR -  Error determining best reference for 'pars-sourcecatalogstep'  =   Unknown reference type 'pars-sourcecatalogstep'
2024-06-11 16:01:07,672 - CRDS - ERROR -  Error determining best reference for 'pars-mosaicpipeline'  =   Unknown reference type 'pars-mosaicpipeline'

Although these are labeled ERROR they are what is expected when a pars-* is not available on crds.

Regression tests show no failures: https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/824/

Fixes https://github.com/spacetelescope/romancal/issues/1116 Fixes https://github.com/spacetelescope/romancal/issues/1111 Fixes RCAL-777

Checklist

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 79.36%. Comparing base (48c05f8) to head (96c36dd).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1270 +/- ## ========================================== + Coverage 79.30% 79.36% +0.06% ========================================== Files 117 117 Lines 8065 8079 +14 ========================================== + Hits 6396 6412 +16 + Misses 1669 1667 -2 ``` | [Flag](https://app.codecov.io/gh/spacetelescope/romancal/pull/1270/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | Coverage Δ | | *Carryforward flag | |---|---|---|---| | [nightly](https://app.codecov.io/gh/spacetelescope/romancal/pull/1270/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | `62.77% <ø> (-0.02%)` | :arrow_down: | Carriedforward from [48c05f8](https://app.codecov.io/gh/spacetelescope/romancal/commit/48c05f8cad6e6a191ffad0eb4ee3050b21f6adfa?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | *This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

stscieisenhamer commented 3 weeks ago

Some questions:

First, how does this "enable" parameter reference files? From the above, the implication is that parameter reference files are json files. However, at least for jwst, parameter reference files are asdf files.

This is also implying that stpipe parameter reference mechanism is not in the core stpipe. Is this true and if so, why is the implementation different between roman and jwst?

There may be need for some more discussion, because of the potentially related CCD-1465.

braingram commented 3 weeks ago

Some questions:

Thanks for taking a look and for the link to the CCD ticket. This isn't quite ready for review and I wasn't sure if the infrastructure is ready for these changes. I started the PR because I was looking at a different issue and fixed _datamodels_open as a side-effect.

First, how does this "enable" parameter reference files? From the above, the implication is that parameter reference files are json files. However, at least for jwst, parameter reference files are asdf files.

It allows Step._datamodels_open to open an association file (stored as json or yaml). The foo.json mentioned above is the association file not the parameter file. Attempting to open an association fails (as noted above) with romancal (it does not fail with jwst, that's the bug fixed here, see the linked issues above).

This is also implying that stpipe parameter reference mechanism is not in the core stpipe. Is this true and if so, why is the implementation different between roman and jwst?

The parameter reference fetching is all handled by stpipe, the same as with jwst.

There may be need for some more discussion, because of the potentially related CCD-1465.

Thanks for the link. This PR is fixing a bug that will allow the parameter file to be fetched whereas that looks to be related to the format of the parameter file (this PR doesn't change anything about the format).

schlafly commented 3 weeks ago

Looks good to me; certainly will be glad not to need --disable-crds-steppars anymore. @ddavis-stsci , I remember you had thoughts here; care to review?

braingram commented 3 weeks ago

Thanks! I requested @stscieisenhamer as another reviewer as he had some questions about these changes.

braingram commented 2 weeks ago

@stscieisenhamer any more questions on this PR?