Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make config template input-type independent. #393

Merged
merged 24 commits into from
Aug 5, 2023

Conversation

cwwalter
Copy link
Member

@cwwalter cwwalter commented Aug 1, 2023

This PR removes specifics of instance catalogs from the imsim-config.yaml. It is now input method independent.

It creates two new config files that use imsim-config.yaml as a template: imsim-instcat.yaml and insim-skycat.yaml. Eventually, with a new version of GalSim these will themselves become templates. But, nested templates are not yet implemented. So, for now, they serve as a template file for copying. The user copies them to a working area and adds the information they want to override at the bottom of the file below the template information. As we create new input methods we can add new files.

The files in config are not meant to be run. They are only config templates. The "examples" directory now contains imsim-user-instcat.yaml and imsim-user-skycat.yaml. These files reproduce the original functionality of the runnable config files.

Since I pulled out the (e.g)

gal:
    type: InstCatObj

stamp.world_pos:
    type: InstCatWorldPos

from the imsim-config file, this willl break things that don't set them as in the config templates for each input type. Will need to see what if anything breaks in the CI when I submit this.

Thoughts welcome.

Need to wait for GalSim v2.5.
These are templates that the user can copy and add to for
each instance type.

With GalSim 2.5 these will become templates.
Including adding a new skycatalog template.
Make clear what the file is for.
Can't redefine if not used before.  But, careful not to wipe
out whole section.
Until GalSim supports nested templates, manually copy the information
into the config files. Also, rename files and register them with the
template system.
@cwwalter
Copy link
Member Author

cwwalter commented Aug 2, 2023

@rmjarvis Folllowing @jchiang87 's suggestion. For now I am just manually copying all of the original template information into the top of (e.g.) imsim-config-instcat.yaml. When the new version of GalSim is being used generally, we will replace that with a template call. So the template file looks like this:

[content of imsim-config.yaml]

################################################################
#  Above is template material.
#
#  Make changes specific to this input method below.
#  When GalSim allows nested templates we will replace this with a
#  template call
################################################################

input.instance_catalog:
    # This should be overridden below or on the command line.
    file_name: default_catalog_file.txt
    sed_dir: $os.environ.get('SIMS_SED_LIBRARY_DIR')

... more overrides...

But when I try to use this in example/imsim-user-instcat.yaml. with

# Get most of the configuration from the imSim config-template
# for instance catalogs.
template: imsim-config-instcat

input.instance_catalog.file_name:  SOME_FILENAME
...

It doesn't work. I just get

galsim.errors.GalSimConfigError: Unable to parse extended key input.instance_catalog.file_name.  Field instance_catalog is invalid.

It is ignoring my override at the end of the template file. Is this a YAML issue of some sort? Should this work?

It seems appending YAML elements in . notation doesn't work.
So, for now, manually insert the elements in the yaml sections.

This is dangerous! Hopefully this will be replaced by a nested template
soon.  We have to be careful to not let config files get out of sync.
Now nothing should be using the core imsim-config.yaml file.

Also, update two tests to use the instcat config file.
@cwwalter
Copy link
Member Author

cwwalter commented Aug 2, 2023

For now to get something to work I am manually inserting the yaml elements into a copy of the config section in each config file. This is dangerous... Hopefully this will be replaced by a nested template soon.

We have to be careful to not let config files get out of sync. Now nothing should be using the core imsim-config.yaml file. Everything (that uses a file input) should use a specific input type template.

@cwwalter
Copy link
Member Author

cwwalter commented Aug 2, 2023

OK this now passes all tests and should be exactly equivalent to before in usage.

When the nested templates work in GalSim, we will replace the pasted common yaml int he config files with a imsim-config template call and override what is needed in the specific input config. This will be invisible to users, a completely internal change.

@cwwalter
Copy link
Member Author

cwwalter commented Aug 2, 2023

I think Mike is on vacation this week. Could you review Jim?

Copy link
Collaborator

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

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

Most of my change requests are stylistic, so not totally essential, but I think would clarify things for new users. One important change, however: If the yaml files in the config folder are supposed to be templates and not run directly, I'd suggest moving the flat*yaml files to the examples folder (and update the CI yaml).

config/imsim-config-instcat.yaml Outdated Show resolved Hide resolved
config/imsim-config-instcat.yaml Outdated Show resolved Hide resolved
config/imsim-config-skycat.yaml Show resolved Hide resolved
config/imsim-config.yaml Show resolved Hide resolved
examples/imsim-user-skycat.yaml Outdated Show resolved Hide resolved
examples/imsim-user-skycat.yaml Outdated Show resolved Hide resolved
imsim/templates.py Show resolved Hide resolved
@cwwalter
Copy link
Member Author

cwwalter commented Aug 4, 2023

Thanks Jim. I also moved the flat files to examples as you suggested.

Copy link
Collaborator

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

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

Thanks, Chris. LGTM.

@cwwalter
Copy link
Member Author

cwwalter commented Aug 4, 2023

I think this is ready to merge. @rmjarvis when you get a chance could you either add anything new or clear your request for a change (now discussed in #397?).

@cwwalter cwwalter merged commit c349d96 into main Aug 5, 2023
3 checks passed
@cwwalter cwwalter deleted the u/cwalter/input-templates branch August 5, 2023 04:31
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.

3 participants