search

NCAR / ccpp-framework

Common Community Physics Package (CCPP)
http://www.dtcenter.org/community-code/common-community-physics-package-ccpp/
Other
26 stars 63 forks source link

Constituent updates #468

Closed peverwhee closed 1 year ago

peverwhee commented 1 year ago

Summary

Incorporate constituent mods into capgen; also any lingering updates/fixes/tests from @gold2718 's capgen fork

Description

Flesh out constituent objects, add necessary interfaces and metadata

User interface changes?: Not that I know of...

addresses #282 addresses #380

Testing:

All unit tests have been updated to reflect changes and pass except for the unit conversion test that is expected to fail

climbfuji commented 1 year ago

I won't be able to review this until June 20 or even later, so please proceed with the approvals of the other two reviewers.

nusbaume commented 1 year ago

Just FYI that after discussing it with NCAR scientists the standard names being brought in by this PR will be updated soon to try and shorten their names, specifically removing the use of the word array in the name, so I might recommend holding off on reviewing the standard names until then. Also there might be an additional constituent property that we (NCAR) will want to add to the constituents properties object. I'll open a PR to Courtney's branch if/when these changes are ready to be commited. Thanks!

gold2718 commented 1 year ago

NCAR scientists the standard names being brought in by this PR

The only standard names in the CCPP Framework should be standard, non-physical quantities such as ccpp_error_message or horizontal_dimension. The standard names in this PR are (or should be) only in unit tests and do not really need to be changed. I vote not making the change now (unless they are somehow in the main codebase in which case they should be removed).

peverwhee commented 1 year ago

Capturing the result of conversations with @goldy and @nusbaume : the changes to standard names that are going to be needed in this PR aren't physical quantities, but instead changes to the constituent-specific standard names in ccpp_constituent_prop_mod.meta (removing "array")

dustinswales commented 1 year ago

@peverwhee @mkavulich @climbfuji @gold2718 There are several things rolled into this PR and I'm finding it a bit hard to review. We have at least four Capgen innovations here.

My preference would be to silo these changes into focused smaller focused PRs. Thoughts?

peverwhee commented 1 year ago

@dustinswales Fair point. I will separate them into their requisite parts and ultimately close this PR