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

Add branch specific namelist settings #43

Merged
merged 4 commits into from
Mar 27, 2023
Merged

Conversation

SeanBryan51
Copy link
Collaborator

This change allows for users to add branch specific namelist settings in the config file that get "patched" to the base namelist settings used for both branches. These additional settings are set by specifying an optional patch key in each realisation. Currently, this change supports the ability to add a patch to both realisations.

Addresses issue #29

This change allows for users to add branch specific
namelist settings in the config file that get "patched"
to the base namelist settings used for both branches.
These additional settings are set by specifying the
`patch` key in each realisation. Currently, this change
supports the ability to add a `patch` to both
realisations.

Addresses issue #29
The user should not specify 'cable' in the top level of the
branch patch dictionary. This is consistent with how science
configurations are specified in site_configs.yaml.

Add a full task setup test case that asserts the branch patch
is written correctly.
@SeanBryan51 SeanBryan51 requested a review from ccarouge March 20, 2023 04:42
@SeanBryan51 SeanBryan51 linked an issue Mar 20, 2023 that may be closed by this pull request
@SeanBryan51
Copy link
Collaborator Author

@ccarouge I've linked issue #29 so that this PR will close the issue. I know a lot of the discussion was about what we do with the model output, but if there were any other requirements for the issue that I have forgotten to add please let me know

Copy link
Member

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

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

One comment but with impact on a few parts of the code and the tests.

benchcab/task.py Outdated
Comment on lines 175 to 176
# the user should not specify 'cable' in the top level of the patch dictionary
self.patch_namelist_file({'cable': self.branch_patch}, root_dir=root_dir)
Copy link
Member

Choose a reason for hiding this comment

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

This is going to break if we change the namelist file to have more sections beyond cable. Should we instead ask the input to be {'cable': {......}}?

The current approach allows for an easier setup in the config file.

It is easy to modify the benchcab code later on if we go that way but it also means we would introduce a breaking change and make previous config.yaml files incompatible. Limiting reproducibility. And people would then have to learn the new format.

I think I would change now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I think having to include {'cable': {......}} in the patch is a good idea. Should we do the same for the science configurations for consistency? For example, in the site_configs.yaml

sci0: {
    cable_user: {
        GS_SWITCH: "medlyn",
        FWSOIL_SWITCH: "Haverd2013"
    }
}

would become

sci0: {
    cable: {
        cable_user: {
            GS_SWITCH: "medlyn",
            FWSOIL_SWITCH: "Haverd2013"
        }
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Indeed we need the same in the science configurations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have made the changes for the patch key: see 2006d9c. I have added a separate issue for the changes for science configurations (see #46) as I prefer to make these changes once this PR and #44 are merged in.

Specify `cable` in the top level of the patch dictionary.
This prevents the code from breaking if we add more
namelists beyond `cable` to the namelist file in the future.

Addresses comment by @ccarouge
#43 (comment)
@ccarouge ccarouge merged commit ad3727c into master Mar 27, 2023
@SeanBryan51 SeanBryan51 deleted the 29-new-dev-feature branch March 27, 2023 09:38
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.

Handle the case for a new feature introduce by developer
2 participants