NCAR / ccpp-framework

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

Bugfix for auto transforms #604

Closed dustinswales closed 2 weeks ago

dustinswales commented 1 month ago

Description

Modifications made to handle local variables needed for variables transforms at the Scheme level. Previously the Group would manage the local variable

Fixes #594

In response to the scenarios in #594:

Scenario 1

If a variable is used in two different schemes and:

  1. The first scheme has the same units as the host variable.
  2. The second scheme has different units than the host variable.

The Error was replicated and fixed by removing a bugfix at line 941 in var_props.py.

Scenario 2

On the flip side, if:

  1. The first scheme has different units than the host variable.
  2. The second scheme has the same units as the host variable.

Output

Generated code for scenario number 2 (where the correct units are used):

! Compute reverse (pre-scheme) transforms
ps_local(:) = 1.0E-2_kind_phys*ps(:)
! Call scheme
call cld_liq_run(ncol=ncol, timestep=timestep, tcld=tcld, temp=temp, qv=qv,              &
              ps=ps_local, cld_liq_array=cld_liq_array, errmsg=errmsg, errflg=errflg)
...
! Call scheme
call cld_ice_run(ncol=ncol, timestep=timestep, temp=temp, qv=qv, ps=ps,            &
              cld_ice_array=cld_ice_array, errmsg=errmsg, errflg=errflg)

Testing

All Capgen tests pass.

dustinswales commented 3 weeks ago

@climbfuji Thanks for the review. I will expand the var_compatibility_test to stress more of the edge cases that @peverwhee brought up.

dustinswales commented 3 weeks ago

@peverwhee It turns out that the real problem was in add_variable, where it was catching an incompatibility when it was actually equivalent. All good now, and unit tests updated to reflect the distinction between VarCompatObj.compat and VarCompatObj.__equiv

climbfuji commented 2 weeks ago

@gold2718 Would you like to review this PR?

dustinswales commented 2 weeks ago

Just typos, otherwise approved!

Personally, I think it's more informative if Scheme, Group, Host, Var , etc..., which are (case sensitive) python classes, be referenced in their native form when commenting on them. Happy to change if others disagree, but "Group" is distinct from "group" in this context.

climbfuji commented 2 weeks ago

Just typos, otherwise approved!

Personally, I think it's more informative if Scheme, Group, Host, Var , etc..., which are (case sensitive) python classes, be referenced in their native form when commenting on them. Happy to change if others disagree, but "Group" is distinct from "group" in this context.

Ok, that's fine with me. The way I read the comments they looked like generic labels (a scheme, a group, ...) to me. Then let's just fix the type for compatibility please.

mkavulich commented 2 weeks ago

@dustinswales The other two PRs have been merged, let me know when this one is updated with the latest changes.

dustinswales commented 2 weeks ago

@mkavulich Updated.

gold2718 commented 2 weeks ago

gold2718

Sorry, been out of commission. Trying to catch up.