Skip to content

Enable a yaml template to reuse in several config files#86

Draft
arunkannawadi wants to merge 17 commits intomainfrom
config_template
Draft

Enable a yaml template to reuse in several config files#86
arunkannawadi wants to merge 17 commits intomainfrom
config_template

Conversation

@arunkannawadi
Copy link
Member

No description provided.

@sidneymau
Copy link
Contributor

sidneymau commented Feb 19, 2026

there are a lot of paths that we probably want to change in here configs

Cargo-cult some structure from imSim for familiarity and reference.
"""
config = galsim.config.ReadConfig(config_file)[0]
# if a yaml template is used, update the config accordingly; otherwise, do nothing
galsim.config.ProcessTemplate(config, base=config)
Copy link
Member Author

Choose a reason for hiding this comment

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

We probably want to use ProcessAllTemplates here.

The default is to point to the paths on DCC. Users running it elsewhere
will have to point to the appropriate directories.
@arunkannawadi
Copy link
Member Author

I have followed the structure adopted in LSSTDESC/imSim to register a template, and find absolute paths more robustly so these can be run from anywhere. @aguinot - for your PR, you just need to update the noise in the template.

@arunkannawadi
Copy link
Member Author

@aguinot
Copy link
Collaborator

aguinot commented Feb 26, 2026

Hey,
I am sorry I think I missed some of the discussions here. Could you briefly explain why there are a _template.py and _meta_data.py files?

@arunkannawadi
Copy link
Member Author

The _templates.py can be used to give a handy name to the full path of the template (config/default.yaml). Otherwise, you need to invoke galsim only from the root directory and will fail if your current directory is different. The _templates.py together with _meta_data.py makes it robust. These are also there in LSSTDESC/imSim, but I have a leading underscore because this is not something that a user will ever need to use directly.

@sidneymau sidneymau self-requested a review February 26, 2026 04:43
@aguinot
Copy link
Collaborator

aguinot commented Feb 26, 2026

Sorry for the basic questions:

  • How do I run the default config? I tried galsim config/default.yaml and I get an error.
  • How do I use the registered template?

@arunkannawadi
Copy link
Member Author

I should change the name of the default.yaml. it is incomplete intentionally and is not runnable. You can see how it is used in was.yaml and even in hack.yaml to see how to use a registered template.

@sidneymau
Copy link
Contributor

sidneymau commented Feb 26, 2026

In the config_template branch of roman_imsim_testdata, the updated hack.yaml uses the template. You can run that like normal (e.g., galsim hack.yaml).

https://github.com/DukeCosmology/roman_imsim_testdata/blob/config_template/hack.yaml

Comment on lines 82 to 91
# These are all by default turned on, but you can turn any of them off if desired:
ignore_noise: True
stray_light: False
thermal_background: False
reciprocity_failure: False
dark_current: False
nonlinearity: False
ipc: False
read_noise: False
sky_subtract: False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sky_subtract: False
# noise:
# # These are all by default turned on, but you can turn any of them off if desired:
# type: RomanNoise
# mjd: { type: ObSeqData, field: mjd }
# stray_light: True
# thermal_background: True
# reciprocity_failure: True
# dark_current: True
# nonlinearity: True
# ipc: True
# read_noise: True
# sky_subtract: True
noise:
type: NoNoise

Update to the new NoiseBuilder

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I am not sure my suggestion will replace all lines 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused what's happening here. I thought the suggestion from earlier was to remove all of the comments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well if you remove the commented stuff you don't see how to specify the noise. If you set the RomanNoise with all other keywords to False you still get background noise. If you want no noise you either have to not set any noise at all or use the explicit NoNoise builder

@aguinot
Copy link
Collaborator

aguinot commented Feb 26, 2026

I think you will probably need to rebase before merging.

@arunkannawadi arunkannawadi marked this pull request as draft February 26, 2026 17:21
@arunkannawadi
Copy link
Member Author

Waiting on CI to be fixed and rebase on main since #66 went in first

@sidneymau
Copy link
Contributor

sidneymau commented Mar 8, 2026

the CI is in a working state now -- I think we can take a look at this again once rebased.

Because there isn't a branch of the same name on roman_imsim_testdata, the "Make Reference Image" job might not work in the current implementation... we should verify that and, if that is the case, figure out how to make it work for this type of change... (or maybe we can run the "End-to-end Test" workflow here and it should work ok as long as the submodule is advanced to the right commit on this branch)

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.

4 participants