NGEET / fates

repository for the Functionally Assembled Terrestrial Ecosystem Simulator (FATES)
Other
105 stars 92 forks source link

Allometry unit test #1186

Closed adrifoster closed 6 months ago

adrifoster commented 7 months ago

Adds a "functional unit test" for the FATES allometry equations, using Fortran code and then a python wrapper script

Description:

Adds some unit test framework for reading in a parameter file, and for using the allometry module Python wrapper scripts build and execute the code, and then plot output.

Collaborators:

lots of help from @billsacks along the way @rgknox

Expectation of Answer Changes:

None - does not affect production code

Checklist

Contributor

Integrator

Documentation

Test Results:

Shouldn't affect anything but will run tests to make sure when we are ready to merge.

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

adrifoster commented 7 months ago

Before this comes in I need to figure out how to get the netcdf and netcdff locations without hard-coding them.

@billsacks said this should be possible with some CIME functions...?

adrifoster commented 7 months ago

Before this comes in I need to figure out how to get the netcdf and netcdff locations without hard-coding them.

@billsacks said this should be possible with some CIME functions...?

okay nevermind it wasn't that hard!

billsacks commented 7 months ago

Awesome work on this @adrifoster ! 🎉

rgknox commented 6 months ago

I ran a test of this on derecho and it is absolutely fabulous, very excited about getting this in the workflow

rgknox commented 6 months ago

@adrifoster and I talked a little about the idea of migrating this work to a new base folder called "unit_tests". This will allow us to to clearly see what is part of this new unit testing framework, and which tests are still in the older deprecated framework in "functional_unit_tests". Any pushback or thoughts on this? (i'd be happy to make the changes to help familiarize myself with the code)

ekluzek commented 6 months ago

@rgknox I'm curious what your directory naming suggestion would look like? I think you are saying to rename this to "unit_tests" which I think is a bad name -- because I think this is more on the integration level testing and hence distinct from "unit tests". So I think there should be a distinction between the two. But, would like to hear more about what you mean here...

adrifoster commented 6 months ago

I think we want to house both unit tests and functional tests here so I'm not sure what to call it I guess. just testing?

rgknox commented 6 months ago

Everything in that "unit_tests" directory, is intended to test "units" of the code. The standard regression tests that we run using run_sys_tests, are also for integrating the code, so calling it integration testing is ambiguous.

ekluzek commented 6 months ago

@rgknox and @adrifoster thanks for the discussion. @rgknox for "integration testing" I'm going for a specific meaning of the term in Software Engineering. This definition from the UCAR SEA "Integration testing builds upon unit testing by examining interactions between larger-scale software components, packages, and modules rather than individual functions and methods". You are right we also can use the term "Integration testing" to be the set of different testing that needs to happen to integrate changes into the code. In that case it usually means the different test suites that are run to make a tag (which will include different categories: unit and system tests with now the addition of functional).

After thinking about it I would categorize @adrifoster new additional as functional, because it's about exploring the scientific behavior of allometry. The fact that it has plotting, rather than pass/fail also tips it toward that direction. In this case it also happens to just explore a single unit of code, which is why @adrifoster is using both functional and unit in the name.

See this discussion...

https://github.com/ESCOMP/CTSM/discussions/2508

ekluzek commented 6 months ago

But, beyond philosophy the bottom line is how to structure and name this? I big contribution here is the build system that @adrifoster brings in. And that build system can likely work for: unit, functional, and integration testing (by the definiton above). Which actually is a great benefit.

But, in practice a developer is going to want to be able to pick between those options -- because of speed. Unit tests are incredibly quick so for them running the entire suite is probably good. But, if you also get the functional tests -- and you aren't going to look at plots -- time is wasted. The functional tests mean you want to examine plots and do extra thinking -- so they should be done separately, and likely you want to pick which ones to run. If Integration tests are added, they may very well take more time so they'll again be something you want to do separate from the others.

So that said. I think we want a directory or file naming structure that allows a distinction between the types. For the functional test I suggest removing "unit" from the name, because the important characteristic is the functional part, meaning you know you'll be looking at plots.

adrifoster commented 6 months ago

Okay I updated this so now we can run the unit tests here too, though right now we can't say run "functional" vs. "unit" tests. That can come in now or in a future PR.

glemieux commented 6 months ago

Regression testing against fates-sci.1.76.1_api.35.1.0-ctsm5.2.004 shows all expected tests passing b4b, as expected. Note that this was tested in tandem with #1191

results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1191-1186-fates