Skip to content

Conversation

@xaviernogueira
Copy link
Contributor

@xaviernogueira xaviernogueira commented Dec 1, 2023

Closes #20

Changes:

  • Changes to TSM process functions to align with our QA/QC Excel sheet / hec-ras.
  • Fully passing test suite at a 0.0000001 (1.0e-7) significance level.
  • A handy script for debugging.

WIP - Next Steps:

  • Fix our answers to account for the scale factor applied in the QA/QC Excel provided by @imscw95 .
  • See how low we can get the tolerance factor and still pass all tests.

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!

@xaviernogueira xaviernogueira added tsm Specific to TSM module tests Relating to our pytest suite labels Dec 1, 2023
@xaviernogueira xaviernogueira linked an issue Dec 1, 2023 that may be closed by this pull request
4 tasks
@xaviernogueira xaviernogueira changed the title TSM tests are all passing! (WIP) TSM tests are all passing! Dec 1, 2023
@codecov
Copy link

codecov bot commented Dec 1, 2023

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@aufdenkampe aufdenkampe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xaviernogueira, this looks good.

Thanks for your clear explanation of the the two approaches considered to model testing (i.e. comparing to RAS-1D-WQ outputs), and your reasons for selecting the pattern that aligns with your existing pytests. That makes sense to me.

@aufdenkampe
Copy link
Member

@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
Copy link
Contributor Author

xaviernogueira commented Dec 1, 2023

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

  • Change the mf_cp_water function in TSM to numpy select so we can use numba.
  • Debug the remaining three failed tests.

@xaviernogueira
Copy link
Contributor Author

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
Copy link
Contributor

imscw95 commented Dec 4, 2023

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
Copy link
Contributor Author

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

@xaviernogueira
Copy link
Contributor Author

@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

@xaviernogueira xaviernogueira marked this pull request as ready for review December 4, 2023 23:18
@xaviernogueira xaviernogueira merged commit 937664c into main Dec 4, 2023
@xaviernogueira xaviernogueira deleted the 20-move-tsm-calculation-tests-to-the-pytest-directory-get-them-passing branch December 4, 2023 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Relating to our pytest suite tsm Specific to TSM module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move TSM calculation tests to the pytest directory -> get them passing

4 participants