EcohydrologyTeam / ClearWater-modules

A collection of water quality and vegetation process simulation modules designed to couple with water transport models.
MIT License
5 stars 1 forks source link

TSM tests are all passing! #58

Closed xaviernogueira closed 11 months ago

xaviernogueira commented 11 months ago

Closes #20

Changes:

WIP - Next Steps:

Discussion of an alternative approach: @aufdenkampe we talked about solving all tests at once using an extra xarray.Dataset dimension. While in a notebook environment, I would agree this is the best way to verify calculations, in pytest you would need to make a fixture of the master xarray.Dataset, alter along using hardcoded values, then read slices of that into EnergyBudget(hotstart_dataset=dataset) which is the non-default pattern. Then in unique functions for each check, you would have your assert statements, as in pytest you want unique functions per test for helpful feedback and to avoid early exceptions. I decided to not pursue this because:

  1. I already had a strong approach working, and I was looking to triage my time.
  2. But mainly it separates the altered inputs from the answers, which makes things more confusing (I.e., scrolling up and down). A solution here is to include an assert statement that checks that the input value is changed, which allows the user to infer that it the only value that is altered. But I prefer the straightforward approach of just a fresh init for each test, no inference required!
codecov[bot] commented 11 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4459156) 38.41% compared to head (b1c9997) 38.53%. Report is 3 commits behind head on main.

Files Patch % Lines
src/clearwater_modules/tsm/processes.py 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #58 +/- ## ========================================== + Coverage 38.41% 38.53% +0.11% ========================================== Files 33 33 Lines 1161 1160 -1 ========================================== + Hits 446 447 +1 + Misses 715 713 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

aufdenkampe commented 11 months ago

@xaviernogueira, with this PR, I think it's time to issue a release!

Let's title it: "v0.2.0: OOP refactor; TSM fully functional" or something like that.

xaviernogueira commented 11 months ago

@aufdenkampe @imscw95

Update: All tests pass down to a tolerance of +/- 0.0000001 except three:

======================================= short test summary info =======================================
FAILED tests/test_5_tsm_calculations.py::test_changed_water_temp_c - assert 39.999152040147095 ± 4.0e-06 == 39.99618297
FAILED tests/test_5_tsm_calculations.py::test_changed_air_temp_c - assert 19.999991244551833 ± 2.0e-06 == 19.99999407
FAILED tests/test_5_tsm_calculations.py::test_changed_wind_b - assert 19.999980923751853 ± 2.0e-06 == 19.99995768
==================================== 3 failed, 12 passed in 9.16s =====================================

As you can see, none fail that bad. I am going to investigate where the math isn't adding up, but things are looking good.

Still to do:

xaviernogueira commented 11 months ago

Debug progress: I fixed all failing tests except 1 which is "changed_wind_b". I am looking into this one, but I suspect it has something to do with the float precision, as wind_b is by default 1.5e-6 and in the wind_function it gets divided by 1,000,000. Therefore there is a term that gets down to 1.5e-12. Our test changes wind_b to 1.0e-6, therefore we are only changing values in the math at e-12 precision. @aufdenkampe

That said, according to https://en.wikipedia.org/wiki/Double-precision_floating-point_format, float64 should have precision to the e-15.

I am going to see if I can adjust the wind function or the test to get it to pass.

========================== short test summary info ==========================
FAILED tests/test_5_tsm_calculations.py::test_changed_wind_b - assert 19.999980923751853 ± 2.0e-06 == 19.99995768
======================= 1 failed, 14 passed in 9.07s ========================
imscw95 commented 11 months ago

Does it work when you set wind_a = .3 and wind_b = 1.5? I believe these values should be on the order of 10^-7 and 10^-6 respectively... so the parameter entry should be in the above suggested range if the code includes the divide by 1000000, or should be entered as 310^-7 and 1.510^-6.

xaviernogueira commented 11 months ago

@imscw95 yes it works fine normally it just doesn't get the right answer when it wind_b changes.

xaviernogueira commented 11 months ago

@imscw95

So I think I figured it out. In the excel there was no division factor, and I realized that you changed multiple wind variables in the "Change win_b" columns when compared to the inputs on the far left.

I simply changed the test to switch wind_b with 1.0, and opposed to 1.0e-6. All tests pass