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

Bug fix in metavar.py: optional var props with a default value need t… #508

Closed climbfuji closed 9 months ago

climbfuji commented 9 months ago

Fix return value for optional variable properties that are not set by the variable and that have a default value (instead of a default function). Credits go to @gold2718 for suggesting this bug fix.

See https://github.com/NCAR/ccpp-framework/issues/509 for a description.

Note due to the updates in this PR, there is a change in which properties show up in places like the datatable.xml file. This triggered a desire to add a CI test for the ccpp_track_variables.py script (see #511).

User interface changes?: No

Fixes https://github.com/NCAR/ccpp-framework/issues/509

Testing:

gold2718 commented 9 months ago

There is a problem with this bug fix! Checking out this branch, the advection test fails (as shown in the CI log). Checking out the source of this branch (NCAR/feature/capgen or eac8bb8), the advection test passes! The only difference is your one line change. Investigating the changes in the generated code, I see two significant changes:

From cld_suite_cap.F90

49c49
<    real(kind_phys)                   :: tcld ! minimum_temperature_for_cloud_liquid
---
>    real(kind_phys),    allocatable   :: tcld ! minimum_temperature_for_cloud_liquid

From test_host_ccpp_cap.F90:

48c48
<    type(ccpp_model_constituents_t)  :: test_host_constituents_obj 
---
>    class(ccpp_model_constituents_t)  :: test_host_constituents_obj

Both of these changes ae wrong! This is one of the issues I was mentioning yesterday with the problem of assuming that a missing optional property should be treated the same as the default value for that property. I would say this needs to be fixed (if possible) before this PR can go in.

As for the description above, perhaps you should change Credits to Blame?

climbfuji commented 9 months ago

48 | class(ccpp_model_constituents_t) :: test_host_constituents_obj | 1 Error: CLASS variable ‘test_host_constituents_obj’ at (1) must be dummy, allocatable or pointer make[2]: [CMakeFiles/TESTLIB.dir/build.make:179: CMakeFiles/TESTLIB.dir/ccpp/test_host_ccpp_cap.F90.o] Error 1 make[1]: [CMakeFiles/Makefile2:85: CMakeFiles/TESTLIB.dir/all] Error 2 make: *** [Makefile:91: all] Error 2


I figured out that it's because now the "polymorphic" optional var prop is also returning `.true.` by default. I'll look into what it is used for (don't know anything about that code) and if `.false.` would be the correct default value for it.
climbfuji commented 9 months ago

@gold2718 I fixed it, turned out to be a really simple bug in the default value definition of the optional var prop polymorphic. Since it's a bool and not a str, the default value needs to be False (Python lingo) and not .false. (Fortran lingo), because if polymorphic evaluates to True if it's given a non-empty string .false.. See https://github.com/NCAR/ccpp-framework/pull/508/commits/294eecf673a3af7486bb6b3f00f0f6eb80d838b0.

gold2718 commented 9 months ago

I figured out that it's because now the "polymorphic" optional var prop is also returning .true. by default. I'll look into what it is used for (don't know anything about that code) and if .false. would be the correct default value for it.

Ow, ow OW (I have to learn to slap my forehead with less force)! In the immortal words of Homer Simpson -- "DOH!" Some day, I will learn to code in one language at a time -- thanks for tracking this down. This PR now solves the code problem, however, I notice that the datatable.xml file is quite different. Could this cause issues? Is it possible to test the ccpp_track_variables.py script?

climbfuji commented 9 months ago

I figured out that it's because now the "polymorphic" optional var prop is also returning .true. by default. I'll look into what it is used for (don't know anything about that code) and if .false. would be the correct default value for it.

Ow, ow OW (I have to learn to slap my forehead with less force)! In the immortal words of Homer Simpson -- "DOH!" Some day, I will learn to code in one language at a time -- thanks for tracking this down. This PR now solves the code problem, however, I notice that the datatable.xml file is quite different. Could this cause issues? Is it possible to test the ccpp_track_variables.py script?

I've never used this and I am running out of time for this week, but I'll try to get to it as soon as I can. Yes, I noted that there are quite some differences in the auto-generated files (order in the Fortran code etc) and found that concerning as well.

gold2718 commented 9 months ago

[ . . . ] I notice that the datatable.xml file is quite different. Could this cause issues? Is it possible to test the ccpp_track_variables.py script?

I've never used this and I am running out of time for this week, but I'll try to get to it as soon as I can. Yes, I noted that there are quite some differences in the auto-generated files (order in the Fortran code etc) and found that concerning as well.

While I'm not concerned by the generated code changes, we could probably fix this by sorting dictionary lists when writing (use statements, variable declarations are two I noticed).

@mkavulich, Do you have a way to test ccpp_track_variables.py? Can it be added to the CI?

mkavulich commented 9 months ago

@gold2718 Good suggestion, unit tests for that script are long overdue. I'll work on it this week.

dustinswales commented 9 months ago

@mkavulich Did you make any progress with CI'ing the ccpp_track_variables.py script? (If not, then I think we should merge this and move on. We could add that test at a later time) @gold2718 @DomHeinzeller Thoughts?

gold2718 commented 9 months ago

@mkavulich Did you make any progress with CI'ing the ccpp_track_variables.py script? (If not, then I think we should merge this and move on. We could add that test at a later time) @gold2718 @DomHeinzeller Thoughts?

I suggest opening an issue to address ccpp_track_variables.py checking and CI. Then this can be merged. It might be nice to also clean up the PR description with the confusion about why the tests failed at first (the code change in this PR turned a None return from get_prop_value into True because .false. is True, not False).

dustinswales commented 9 months ago

@gold2718 See https://github.com/NCAR/ccpp-framework/issues/511

mkavulich commented 9 months ago

Sorry, I didn't realize I was holding up this PR. You can go ahead and merge, my plan was to implement that test in the main branch anyway.

climbfuji commented 9 months ago

I updated the PR description to clean it up and add a note about the properties, as requested. I'll go ahead and merge. Thanks for your feedback and reviews.