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

[climbfuji]

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 #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 #509

Testing:

• test removed: n/a
• unit tests: added 3 doctests in metavar.py
• system tests: I ran cd test && ./run_fortran_tests.sh on my macOS and all tests passed (noting that var_action_test is skipped on the feature/capgen branch because of the failing unit conversion tests)
• manual testing: ran the doctests on metavar.py as described in the issue
• CI: all tests pass (after adding 294eecf)

[gold2718]

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]

• Before, CI failed: Interestingly, the capgen CI unit tests failed, but I am having a hard time seeing how my change could be responsible for this (see https://github.com/NCAR/ccpp-framework/actions/runs/6541756913/job/17763731009?pr=508). However, I tried feature/capgen without this change in Do not merge (trigger CI for feature/capgen branch) #510, and the tests passed. I will investigate tomorrow ...

[ 83%] Building Fortran object CMakeFiles/TESTLIB.dir/ccpp/test_host_ccpp_cap.F90.o
/home/runner/work/ccpp-framework/ccpp-framework/test/at_build/ccpp/test_host_ccpp_cap.F90:48:66:

   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]

@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 294eecf.

[gold2718]

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]

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]

[ . . . ] 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]

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

[gold2718]

It should probably be documented somewhere that this will change which properties show up in places such as the datatable.xml file.

[dustinswales]

@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]

@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]

@gold2718 See #511

[mkavulich]

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]

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.