search

NOAA-OWP / snow17

Other
4 stars 10 forks source link

Added unit test and unimplemented BMI 2.0 functions #15

Closed zhengtaocui closed 2 years ago

zhengtaocui commented 2 years ago

TYPE: Feature enhancements and bug fixes

KEYWORDS: Unit test, BMI 2.0, get_value_at_indices, get_value_ptr, runtime warnings and errors, the check=all compiler option

SOURCE: Zhengtao Cui, NOAA NWS OWP

DESCRIPTION OF CHANGES:

1) Implemented these get_value_at_indices and get_value_ptr to return BMI_FAILURE to be compliance with BMI 2.0 standards. get_value_at_indices_int get_value_at_indices_float get_value_at_indices_double set_value_at_indices_int set_value_at_indices_float set_value_at_indices_double

get_value_ptr_int

get_value_ptr_float

get_value_ptr_double

2) Added the unit test code. The unit code was created based on the unit test code in https://github.com/NOAA-OWP/noah-owp-modular/

3) Fixed the warnings and errors at runtime when it is compiled with the -fcheck=all option

At line 150 of file ..//src/share//runSnow17.f90 Fortran runtime warning: An array temporary was created At line 152 of file ..//src/share//runSnow17.f90 Fortran runtime warning: An array temporary was created Finalizing... At line 195 of file ..//src/share//runSnow17.f90 Fortran runtime error: Index '4' of dimension 1 of array 'model' above upper bound of 3

ISSUE: There are compiling warnings about unused dummy variables.

TESTS CONDUCTED: Tested with the unit test code. All tests passed.

NOTES: Tested only with the gfortran compiler on Hera. Need to be tested with an Intel compiler.

andywood commented 2 years ago

Looks good and glad these changes fixed the problem. I have one suggestion and one request: -- suggestion: add a comment line where the arrays are defined to note why the order is important (ie fortran arrays are column-major which means the first index is contiguous in memory -- thus is can be passed to subroutines this way without creating temp. arrays. That's good to know and will avoid anyone trying to change it back in the future. -- request: can we move and rename the /snow17/src/test/ directory to /snow17/test_cases/unit/ ? ---- there is already a /test_cases/ directory so it may be confusing to have a second /test/ directory, with its own Makefile. The regular Makefile is in /snow17/build/.

One other comment -- I have been using the develop branch to make changes, before periodically pushing those to the master branch (which would be done for a new tag or release). I somewhat prefer that rather than developing straight on the master branch. Perhaps we can do this in the future? We'll need to to remember to sync this down into the develop branch after merging it.

Note, I'm by no means an expert on gitHub or the code structure conventions that OWP wants to use, so please feel free to push back on these suggestions.

zhengtaocui commented 2 years ago

Andy, Thanks for the comments. I have added comments about the row/column orders for the two variables. I also moved the test directory from src to test_cases/unit_test. Please review.

Regarding the branches, I don't know what the protocols are for Nexgen. I can use the dev branch in the future development. However, my impression is that the master or trunk always contains the latest development. Along the trunk, there are branches or tags for revisions that needs to be stable. But I am not an expert about github.

andywood commented 2 years ago

Looks good, thanks Zhengtao ...