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

Unit conversion bug when variable is used by two different schemes (with different units) #594

Closed
peverwhee opened this issue Sep 30, 2024 · 10 comments
Labels

Comments

@peverwhee
Copy link
Collaborator

Description

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.

It produces this error:

Invalid new variable, <variable>, incompatible  between <variable>,

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.

The framework will convert the variable to pass into the first scheme, but not convert it back for the second scheme (thus using the wrong units)

Steps to Reproduce

  1. Modify ps in cld_ice.meta in the advection test to have units of "hPa", run the tests and observe the error.
  2. Remove your modification to cld_ice.meta. Modify ps in cld_liq.meta in the advection test and inspect the generated code to see the wrong units passed into cld_liq

Output

Generated code for scenario number 2 (where the wrong 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_local,            &
              cld_ice_array=cld_ice_array, errmsg=errmsg, errflg=errflg)
@peverwhee peverwhee added the bug label Sep 30, 2024
@climbfuji
Copy link
Collaborator

is this also a problem with prebuild or just with capgen?

@peverwhee
Copy link
Collaborator Author

@climbfuji good question!

I ran a brief test with prebuild and it looks like it works as expected for Scenario 1, but Scenario 2 results in an incorrect conversion before the second scheme. For example, if you have variable unit_conversion_var with units of km in the host model and then Scheme A has units for m (different from host) for that variable, while Scheme B has units of km (same as host) for that variable, you get code that looks like:

tmpvar_1 = 1.0E+3_kind_phys*unit_conversion_var(one:nx)
call schemeA(errmsg,errflg,tmpvar_1)
unit_conversion_var(one:nx) = 1.0E-3_kind_phys*tmpvar_1
...
tmpvar_1 = 1.0E+3_kind_phys*unit_conversion_var(one:nx)
call schemeB(errmsg,errflg,tmpvar_1)
unit_conversion_var(one:nx) = 1.0E-3_kind_phys*tmpvar_1

When you should get code that looks like:

tmpvar_1 = 1.0E+3_kind_phys*unit_conversion_var(one:nx)
call schemeA(errmsg,errflg,tmpvar_1)
unit_conversion_var(one:nx) = 1.0E-3_kind_phys*tmpvar_1
...
call schemeB(errmsg,errflg,unit_conversion_var)

NOTE: I modified the optional arguments prebuild test for this, so the fact that the conversions are being done on optional arguments may be part of the problem!

@climbfuji
Copy link
Collaborator

Thanks for testing. This is not what I expected (from my vague memory of how this is implemented)!

@dustinswales
Copy link
Collaborator

@peverwhee I think I know what's going on. I had done the same thing for optional args as for unit-transformations, and I fixed the optional-arg bug in my SCM work.

@dustinswales
Copy link
Collaborator

dustinswales commented Oct 7, 2024

@climbfuji @peverwhee I can confirm that this is an issue with Prebuild.

Here are snippets from a cap where I am calling two radiation schemes, first RRTMG w/o unit conversions, and then a second ML based radiation that requires unit conversions. Look at cld_ref_Lliq and tmpvar_1

Local declaration for unit-transfroms (2D)
Screenshot 2024-10-07 at 10 47 44 AM

Input to scheme A, no unit conversion:
Screenshot 2024-10-07 at 10 49 14 AM

Input to scheme B, unit conversion (wrong allocation)
Screenshot 2024-10-07 at 10 47 34 AM

@climbfuji
Copy link
Collaborator

@dustinswales Thanks. I am confused though what exactly the problem is (cld_ref_liq / tmpvar_1; "wrong allocation"). What is wrong? Is the problem one:physics%Model%levs vs one:physics%Interstitial%lmk?

@dustinswales
Copy link
Collaborator

@dustinswales Thanks. I am confused though what exactly the problem is (cld_ref_liq / tmpvar_1; "wrong allocation"). What is wrong? Is the problem one:physics%Model%levs vs one:physics%Interstitial%lmk?

@climbfuji It's trying to allocate a rank 3 array, when it's declared as rank 2.

@climbfuji
Copy link
Collaborator

Oh. That's a different issue though than what is described above (#594 (comment)). It's still a bad bug but should probably have its own issue.

@climbfuji
Copy link
Collaborator

As shown in #600, ccpp_prebuild.py does not suffer from this particular bug (but from another #598 which #600 fixes).

mkavulich pushed a commit that referenced this issue Oct 22, 2024
… are slices of a n+1 dimensional array have wrong allocation) (#600)

## Description

1. Bug fix in `scripts/mkstatic.py`: define `dim_string_allocate` so
that temporary (group cap) variables of rank `n` that correspond to a
slice of an `n+1` dimensional array have the right dimensions in the
assignment calls and subroutine call lists. See the inline documentation
added in `mkstatic.py` around line 1476 for more information.

2. Addition of test `test_prebuild/test_unit_conv` to test for the above
and to test for the `capgen` issue reported in
#594 (Unit conversion bug
when variable is used by two different schemes with different units).
Note that `ccpp_prebuild.py` did not suffer from this bug, but I wanted
to make sure that we test for it.

3. In the development of the new test `test_unit_conv`, I discovered
that we should declare all incoming host variables in the group caps as
`target`. This is because it is tricky in prebuild to determine that a
parent variable `bar` needs to have the `target` attribute in case a
slice of it has the active attribute. This is a bit of an edge case, but
I believe this additional attribute is safe. I will run full UFS RTs
with this PR to check if the results are b4b or if the addition of
`target` causes the compiler to optimize differently.

4. Minor updates to
`test_prebuid/{test_blocked_data,test_chunked_data}`: fix wrong name of
subroutines in diagnostic error messages, set thread number and thread
counter to safe values when running over the entire domain.
mkavulich pushed a commit that referenced this issue Nov 7, 2024
Modifications made to handle local variables needed for variables
transforms at the Scheme level. Previously the Group would manage the
local variable

Fixes #594 

Co-authored-by: Dom Heinzeller <dom.heinzeller@icloud.com>
@peverwhee
Copy link
Collaborator Author

fixed in #604

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants