romanisim.models

[yuedong0607]

This PR integrates the galsim.roman functionality into romanisim and enables direct access to both CRDS and the Roman Technical Information repository:

• Introducing a new module romanisim.models
• Replacing corresponding galsim.roman calls throughout: romanisim codebase, executables, and unit tests.

Current status:

• - passed all existing unit tests
• - passed two end-to-end tests for making l1 and l2 images:

   romanisim-make-image --level 1 --usecrds --psftype epsf --rng_seed 42 out_l1.asdf
   romanisim-make-image --level 2 --usecrds --psftype epsf --rng_seed 42 out_l2.asdf

• The outputs are consistent with previous behavior, differing only at the level of background modeling.
• Full agreement is achieved when models.bandpass and models.background calls are reverted to the original galsim.roman implementations.

TODO (near term):

• Decide on a strategy for handling the hardcoded path to the locally cloned roman-technical-information repository.
• Add unit tests for romanisim.models
• Add romanisim.models.flat
• Improve docstrings and example notebook

[schlafly]

Re

• Decide on a strategy for handling the hardcoded path to the locally cloned roman-technical-information repository.

I'm going to start a thread with Rob Zellem about whether he wants to make roman-technical-information pip installable. If he doesn't, I'm going to propose that we just put a fixed version roman-technical-information in romanisim and update it periodically with romanisim releases. Presumably we could also support in romanisim getting data from a non-default roman-technical-information if an environment variable were set?

Do you think that would be a good approach?

[yuedong0607]

Re

• Decide on a strategy for handling the hardcoded path to the locally cloned roman-technical-information repository.

I'm going to start a thread with Rob Zellem about whether he wants to make roman-technical-information pip installable. If he doesn't, I'm going to propose that we just put a fixed version roman-technical-information in romanisim and update it periodically with romanisim releases. Presumably we could also support in romanisim getting data from a non-default roman-technical-information if an environment variable were set?

Do you think that would be a good approach?

Hi Eddie, this sounds good to me. I was using try except blocks for reading files from roman-technical-information so in principle it should run even the path is not valid. But it seems the build tests still complain.

[yuedong0607]

But it seems the build tests still complain.
It seems it has captured only the RuntimeError exception which leads to the complaints. I replaced them with Exception instead.

[codecov]

Codecov Report

❌ Patch coverage is 79.59750% with 294 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.00%. Comparing base (d18949b) to head (aef0088).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
romanisim/models/bandpass.py	66.84%	62 Missing ⚠️
romanisim/models/dark_current.py	34.69%	32 Missing ⚠️
romanisim/models/nonlinearity.py	77.41%	28 Missing ⚠️
romanisim/models/read_noise.py	29.72%	26 Missing ⚠️
romanisim/models/_util.py	20.68%	23 Missing ⚠️
romanisim/models/ipc.py	37.50%	20 Missing ⚠️
romanisim/models/gain.py	35.71%	18 Missing ⚠️
romanisim/models/psf_utils.py	86.15%	18 Missing ⚠️
romanisim/models/saturation.py	35.71%	18 Missing ⚠️
romanisim/models/wcs.py	94.31%	12 Missing ⚠️
... and 10 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #328      +/-   ##
==========================================
- Coverage   89.63%   85.00%   -4.63%     
==========================================
  Files          17       32      +15     
  Lines        2633     3615     +982     
==========================================
+ Hits         2360     3073     +713     
- Misses        273      542     +269     

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

[schlafly]

I had a conversation with Rob Zellem, Eric Switzer, and Josh Schlieder about the roman-technical-information repo. They agreed that we should make it pip-installable. But they also thought that in order to avoid delaying this PR, we could put a copy of roman-technical-information in romanisim temporarily.

I think the way that I would do this would be to introduce (sorry, you may already do this, I haven't actually reviewed!) a function that gives the roman-technical-information directory. It would look for it in an environment variable and fall back to something like romanisim/data/roman-technical-information if not present. Then when roman-technical-information becomes pip installable, you would update that function to instead point to roman-technical-information/data instead with no other changes? They're aiming for pip installability this build but not immediately. Does that make sense for this PR?

[yuedong0607]

I had a conversation with Rob Zellem, Eric Switzer, and Josh Schlieder about the roman-technical-information repo. They agreed that we should make it pip-installable. But they also thought that in order to avoid delaying this PR, we could put a copy of roman-technical-information in romanisim temporarily.

I think the way that I would do this would be to introduce (sorry, you may already do this, I haven't actually reviewed!) a function that gives the roman-technical-information directory. It would look for it in an environment variable and fall back to something like romanisim/data/roman-technical-information if not present. Then when roman-technical-information becomes pip installable, you would update that function to instead point to roman-technical-information/data instead with no other changes? They're aiming for pip installability this build but not immediately. Does that make sense for this PR?

Yes, I think that makes perfect sense. I'll proceed by including a temporary copy of roman-technical-information along with the helper function. I'll also try fixing the errors in the documents that are reported by doctest, not so familiar with this :)

Thanks for the context from the discussion. Great it will be pip-installable.

[schlafly]

Oh, yeah, we hadn't been using doctests in this code base, so you're introducing the first ones and presumably that leads to issues. There's probably a way to turn off doctests; feel free to do that if resolving the infrastructural issues proves challenging.

[schlafly]

Hi Yuedong,

This was a ton of work! Thanks.

I left a few comments in line that we should discuss. Broadly:

• we should think about how the current parameter defaults in parameters.reference_data interact with the new defaults
• it probably makes sense to move some of the logic for grabbing a reference file using either default information or the image model to a separate routine? It looks very similar among the different models.
• is there any documentation on the new data files added in this PR? I'm okay with adding them but I know that I personally can't maintain them; I don't really know where they come from, etc.. Some mixture of documentation & agreement that you're on the hook to maintain those at some level would be appreciated. On the flip side, we're on the hook to make sure we can do releases when you need them for your sims?
• Special treatment of persistence relative to other instrumental effects is because that one needs more than just a reference file?

[schlafly]

How much of this do you think we should keep vs. how much duplicates stuff you have elsewhere now?

[yuedong0607]

The duplication default values for dark, gain, and read_noise have been removed. There're still default coefficients for nonlinearity and the IPC kernel defined in the corresponding modules. Since these are not single scalar values, I don't think they naturally fit in the reference_data dict.

[schlafly]

The old approach would have been to leave them in parameters.py, just not in the reference_data dictionary. That's where ipc_kernel used to be. That has the mild advantage of letting people overwrite via a config file. OTOH pushing it into the models has the advantage of putting the definitions closer to where they're used. Basically this is fine, thanks.

[schlafly]

I'm trying to decide how we conceptually separate romanisim.models.psf and romanisim.models.psf_utils. One approach would be to leave psf.py in the higher directory as "romanisim's special PSF wrapper" and leave this here as "the implementation of the PSF from galsim + technical repo" (though if that made sense we should try to put the zernickes in the technical repo?). I'm open to other thoughts here.

[yuedong0607]

I moved the romanisim.models.psf up to romanisim.psf. I'd support the idea of putting those zernicke coefficient files in the technical repo.

[schlafly]

I had included this as an optional flag. I guess excluding it makes sense because the apply(...) interface only expects one image?

[yuedong0607]

I think that makes sense. If needed, we could implement a separate method in these modules, something like apply_dq().

[yuedong0607]

Hi Yuedong,

This was a ton of work! Thanks.

I left a few comments in line that we should discuss. Broadly:

• we should think about how the current parameter defaults in parameters.reference_data interact with the new defaults
• it probably makes sense to move some of the logic for grabbing a reference file using either default information or the image model to a separate routine? It looks very similar among the different models.
• is there any documentation on the new data files added in this PR? I'm okay with adding them but I know that I personally can't maintain them; I don't really know where they come from, etc.. Some mixture of documentation & agreement that you're on the hook to maintain those at some level would be appreciated. On the flip side, we're on the hook to make sure we can do releases when you need them for your sims?
• Special treatment of persistence relative to other instrumental effects is because that one needs more than just a reference file?

Hi Eddie,

Thanks for the careful review and helpful comments! I'll work through your inline comments shortly. Regarding your general points:

• Agreed. For now, I've just tried to retain all existing default parameters from the three sources: (following the priority order) 1) roman_techinical_information repository; 2) romanisim; 3) galsim.roman
• I agree there is some repetitive across models when gathering reference files. The main complication is that a particular model (e.g, the nonlinearity model) may require more than one reference files along with some pre-processing. So it might be a little difficult for having an universal _get_crds_model() method. But I think it should be easy to have a shared helper routine to centralize the logic for gathering the required CRDS references. Any suggestion on this?
• The newly added data files are all from galsim.roman. I'll ask around see if there's corresponding documentations.
• In roman_imsim, there're some ongoing efforts regarding the up-the-ramp sampling that follow a slightly different logical (e.g., caching a big photon array) compared to the current romanisim implementation. Thus I have not yet attempted to create a unified model for persistence. Additionally, the modeling parameters in roman_technical_information repository appear to differ from those used in both romanisim and galsim.roman. I think maybe we need some further discussion on the details?

[yuedong0607]

Hi @schlafly , do you think there are any additional changes I should make to this branch before merging to main?

Also, would it be okay for me to start removing parameters, bandpass, nonlinearty, and wcs from the romanisim folder? I'll double-check the corresponding version in romanisim.models contain everything needed before doing so.