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

Fixes component naming and version to replace clm4.5 with elm #6276

Merged
merged 3 commits into from
Mar 4, 2024

Conversation

thorntonpe
Copy link
Contributor

@thorntonpe thorntonpe commented Mar 3, 2024

Correct component naming and version to replace clm4.5 with elm in the case description, written to README.case for each created case.

Fixes #6245
Reported-by: Rob Jacob jacob@anl.gov
[BFB]

Correct component naming and version to replace clm4.5 with elm
in the case description, written to README.case for each created case.
see E3SM-Project#6245
Reported-by: Rob Jacob <jacob@anl.gov>
[BFB]
@thorntonpe
Copy link
Contributor Author

Replaces clm4.5 with elm in the case description, written to README.case for each created case.
see #6245
Reported-by: Rob Jacob jacob@anl.gov
[BFB]

@thorntonpe thorntonpe changed the title Correct component naming and version to replace clm4.5 with elm Fixes component naming and version to replace clm4.5 with elm Mar 3, 2024
@thorntonpe thorntonpe added Land bug fix PR BFB PR leaves answers BFB labels Mar 3, 2024
@thorntonpe thorntonpe requested a review from bishtgautam March 3, 2024 17:52
<desc compset="_ELM%[^_]*CNPRDCTCBC" >elm C-N-P, nutirent competition via relative demand, ctc soil cascade with black carbon deposition:</desc>
<desc compset="_ELM%[^_]*CNECACTCBC" >elm C-N, nutirent competition via equilibrium chemistry approximation, ctc soil cascade with black carbon deposition:</desc>
<desc compset="_ELM%[^_]*CNPECACTCBC">elm C-N-P, nutirent competition via equilibrium chemistry approximation, ctc soil cascade with black carbon deposition:</desc>
<desc compset="_ELM50" >elm physics:</desc>
Copy link
Member

Choose a reason for hiding this comment

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

Are "ELM" and "ELM50" really distinct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are "ELM" and "ELM50" really distinct?

I'm not aware of anyone using the ELM50 compsets. I think this could get cleaned up in future revision.

Copy link
Member

Choose a reason for hiding this comment

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

Actually this ELM50 block can be removed. We have no compsets with "ELM50" in them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this ELM50 block can be removed. We have no compsets with "ELM50" in them.

@rljacob do you want this cleaned up now? There are a number of other CLM/ELM naming issues that this PR does not resolve. We were thinking of going back and doing a more complete cleanup after the v3 tag.

Copy link
Member

Choose a reason for hiding this comment

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

I'll remove the descriptions in the push I'm about to make. Other cleanup can wait.

<desc compset="_ELM50%[^_]*SP" >clm5.0 Satellite phenology:</desc>
<desc compset="_ELM50%[^_]*BGC" >clm5.0 bgc (cn and methane):</desc>
<desc compset="_ELM" >elm physics:</desc>
<desc compset="_ELM%[^_]*SP" >elm Satellite phenology:</desc>
Copy link
Member

Choose a reason for hiding this comment

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

These are additive with each one that matches in order so don't put "elm" in any of the others after "elm physics:". If you had ELM%SP, the resulting string would be "elm physics: elm Satellite phenology:" with what you have now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are additive with each one that matches in order so don't put "elm" in any of the others after "elm physics:". If you had ELM%SP, the resulting string would be "elm physics: elm Satellite phenology:" with what you have now.

I think it's ok to have this redundancy. the elm physics and elm satellite phenology are very distinct features.

@rljacob
Copy link
Member

rljacob commented Mar 3, 2024

What testing did you do for these changes?

@rljacob rljacob added this to the v3.0.0 milestone Mar 3, 2024
@rljacob
Copy link
Member

rljacob commented Mar 3, 2024

Actually the CI passed which includes building/running ELM so it should be fine.

@rljacob
Copy link
Member

rljacob commented Mar 3, 2024

This is what ELM%CNPRDCTCBCTOP generates

LND component is elm physics:elm cn:elm C-N-P, nutirent competition via relative demand, ctc soil cascade with black carbon deposition:elm C-N-P, nutrient competition via relative demand, ctc soil cascade with black carbon deposition and subgrid solar radiation parameterization :

@thorntonpe
Copy link
Contributor Author

What testing did you do for these changes?

I only ran an X test to make sure it built correctly.

@thorntonpe
Copy link
Contributor Author

This is what ELM%CNPRDCTCBCTOP generates

LND component is elm physics:elm cn:elm C-N-P, nutirent competition via relative demand, ctc soil cascade with black carbon deposition:elm C-N-P, nutrient competition via relative demand, ctc soil cascade with black carbon deposition and subgrid solar radiation parameterization :

That looks ok to me. Some minor redundancy with the elm cn and elm C-N-P, but not confusing.

@rljacob
Copy link
Member

rljacob commented Mar 3, 2024

@thorntonpe can you give me write permission to your fork? I found a way to remove the redundancy.

@thorntonpe
Copy link
Contributor Author

@thorntonpe can you give me write permission to your fork? I found a way to remove the redundancy.

I'm looking for how to do that. Is it under "collaborators"?

@thorntonpe
Copy link
Contributor Author

@thorntonpe can you give me write permission to your fork? I found a way to remove the redundancy.

I'm looking for how to do that. Is it under "collaborators"?

@rljacob Try now - I added you as a collaborator.

Add capitalization in ELM descriptions and remove redundant use of "ELM".
@rljacob
Copy link
Member

rljacob commented Mar 4, 2024

Yes it worked. Just pushed my changes. There where 2 missing that I added: CNECACNTBC CNPECACNTBC. Please check those descriptions. I did not figure out how to avoid repeating descriptions but the basic water cycle case looks better:
Component LND is ELM with :CN :C-N-P, nutirent competition via relative demand, ctc soil cascade with black carbon deposition :C-N-P, nutrient competition via relative demand, ctc soil cascade with black carbon deposition and subgrid solar radiation parameterization :

@thorntonpe
Copy link
Contributor Author

Yes it worked. Just pushed my changes. There where 2 missing that I added: CNECACNTBC CNPECACNTBC. Please check those descriptions. I did not figure out how to avoid repeating descriptions but the basic water cycle case looks better: Component LND is ELM with :CN :C-N-P, nutirent competition via relative demand, ctc soil cascade with black carbon deposition :C-N-P, nutrient competition via relative demand, ctc soil cascade with black carbon deposition and subgrid solar radiation parameterization :

Thanks Rob. I just noticed that all except the new v3 compset line have nutrient misspelled as "nutirent". Do you want to fix that, or should I?

@rljacob
Copy link
Member

rljacob commented Mar 4, 2024

you can fix it.

Replaced "nutirent" with "nutrient"
[BFB]
@thorntonpe
Copy link
Contributor Author

Spelling fix is done.

rljacob added a commit that referenced this pull request Mar 4, 2024
Correct component naming and version to replace clm4.5 with elm in the
case description, written to README.case for each created case.

Fixes #6245
Reported-by: Rob Jacob jacob@anl.gov
[BFB]
@rljacob rljacob merged commit d6e9af3 into E3SM-Project:master Mar 4, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB bug fix PR Land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ELM case description
4 participants