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 error message if user_nl_blom includes hash comments #451

Merged

Conversation

TomasTorsvik
Copy link
Contributor

Same as #448, applied for the master branch.

@TomasTorsvik TomasTorsvik self-assigned this Dec 12, 2024
@TomasTorsvik TomasTorsvik added the documentation Improvements or additions to documentation label Dec 12, 2024
Copy link
Collaborator

@jmaerz jmaerz left a comment

Choose a reason for hiding this comment

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

Hi @TomasTorsvik , technically approved, but I just realize that one could have potentially handled it also as a comment similar to the lines above (l. 1338) by

line=remove_user_nl_comment(line)
  • not sure, though, whether this would induce other side effects... - similarly for the other cases...

@TomasTorsvik
Copy link
Contributor Author

Hi @TomasTorsvik , technically approved, but I just realize that one could have potentially handled it also as a comment similar to the lines above (l. 1338) by

line=remove_user_nl_comment(line)
* not sure, though, whether this would induce other side effects... - similarly for the other cases...

@jmaerz - True, but the old user_nl_blom files use a different format for setting variables, using a set keyword. One could also search for set, but this could cause issues if a variable name happened to start with set.... If the user_nl_blom file use # comments, probably something else could be wrong as well.

@jmaerz
Copy link
Collaborator

jmaerz commented Dec 12, 2024

Hi Tomas, thanks for clarification. I guess, moving forward in this case is totally fine - from my side, just go ahead with the PR.

@JorgSchwinger
Copy link
Contributor

It's fine for me including this, but I think it is actually not necesarry.

The starting point was that older user_nl_blom files were not compatible with noresm 2.0.9. This was an issue because this model version is still (compatible to) our CMIP6 model, so a user could expect also the old user namelist files to work here.

However moving forward, there is no reason to expect this - format of user_nl_blom has changed (there are also other changes, not only the # vs !...), but that is fine because it is a new model version.

@TomasTorsvik
Copy link
Contributor Author

It's fine for me including this, but I think it is actually not necesarry.

The starting point was that older user_nl_blom files were not compatible with noresm 2.0.9. This was an issue because this model version is still (compatible to) our CMIP6 model, so a user could expect also the old user namelist files to work here.

However moving forward, there is no reason to expect this - format of user_nl_blom has changed (there are also other changes, not only the # vs !...), but that is fine because it is a new model version.

Yes, hopefully this will not have much use, at least when going to NorESM2.5. At the moment this change will make it into the planned release-1.7 as well.

@TomasTorsvik TomasTorsvik merged commit 13e25f1 into NorESMhub:master Dec 13, 2024
2 of 4 checks passed
@TomasTorsvik TomasTorsvik deleted the main-feature_user_nl_file branch December 13, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants