search

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

feature/capgen: add new tests for unit conversions #434

Closed climbfuji closed 2 years ago

climbfuji commented 2 years ago

Description

Add new tests for unit conversions as (1) unit tests and (2) system tests in a separate var_action_test folder. These are testing variables inside a DDT and outside, and with different intents (in, out, inout).

The unit tests all pass, because the unit conversion at variable level (class function compatible) is already implemented. The system tests fail, because capgen does not yet generate the unit conversion code at the suite level.

What's not included?

I was looking for possibilities to add unit tests at the suite_objects.py level, in particular around the analyze function of the scheme class. Implementing tests there in form of doc tests would be complicated and not so much different from the system tests in the new var_action_test folder.

It is also difficult to write unit tests alongside the new system tests, because I don't know how the exact code block should look like in capgen-generated code. Certainly different from the prebuild-generated code!

Additional details

User interface changes?: No

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

Testing: test removed: unit tests: unit tests (doc tests) for testing variable compatibility/unit conversions to metavar.py and var_props.py (these tests pass) system tests: new test directory tests/var_action_test (these tests fail) manual testing:

gold2718 commented 2 years ago

Please do not overload capgen_test, it is already complicated enough.

climbfuji commented 2 years ago

Please do not overload capgen_test, it is already complicated enough.

The other option I can offer is to rsync capgen_test to another directory var_action_test, strip out everything that is not needed for the var action tests and and keep my proposed changes there.

This way we have the same framework for these end-to-end style tests, but with fewer code in each of them. Some code duplication, but maybe still better than overloading capgen_test.

gold2718 commented 2 years ago

The other option I can offer is to rsync capgen_test to another directory var_action_test, strip out everything that is not needed for the var action tests and and keep my proposed changes there.

Yes please! That is the approach I took with the advection_test.

climbfuji commented 2 years ago

The other option I can offer is to rsync capgen_test to another directory var_action_test, strip out everything that is not needed for the var action tests and and keep my proposed changes there.

Yes please! That is the approach I took with the advection_test.

Done. This looks much simpler now, and the tests are failing when comparing the return values to the expected values, so that's good because we expect this.

climbfuji commented 2 years ago

I made the changes as requested, including reducing the tolerance to 1.0E-10. This is to some extend a moot point, because if unit conversions are not implemented the differences are much larger, as ./run_tesh.sh shows:

1) var_action_suite = test_suites(1)
 Checking suite var_action_suite
Error: max diff of phys_state%effrl from expected value exceeds tolerance:    0.2499950E+01 >    0.5000000E-14
Error: max diff of phys_state%effri from expected value exceeds tolerance:    0.7499992E+02 >    0.7500000E-14
Error: max diff of            effrs from expected value exceeds tolerance:    0.9999990E+01 >    0.5100000E-13
 Answers are not correct!
STOP -1

All other tests pass, therefore this is expected.