Algae Module Tests

[kewalak]

32 passing tests for the algae module (test_7_nsm_algae_calculations.py). File test_6_nsm_module.py has one failing test using the increment_timestep() that I have not figured out. Attached is the excel workbook I used to create the test cases. All scenario outputs from the excel book I checked against the RAS WQ outputs.

Manual Calculations for Algae.xlsx

[imscw95]

Thank you Kelsey! Looks great. Couple things that I saw when going over it this morning. Plan to return to it tomorrow morning, but here are my thoughts:

I saw that one error raised by test_6 was with the np.select syntax, so I fixed those. Another error that's being raised is with the math.isnan() and min functions in processes.py. I think using the numpy versions of these functions may work, but when I made those changes (only 3 places in the python file), there was another confusing error that came up, so I reverted the change, to try to make runs in the notebook.

There seems to be an issue running more than one increment timestep now, and the model is having a hard time identifying the time_dim variable. The error suggests using a method such as nearest to help identify the time_dim but I'm confused how to try that. I tried adding a method to this part of the base.py file, but that didn't work. I see you added a timestep variable to be initialized with the model outside of our global variable timestep, but left the global variable timestep in place. Could you explain why you added that timestep variable in the init? Maybe that's better just wondering.

[sjordan29]

Hi @kewalak and @imscw95, I should be able to take a look at this tomorrow. In the meantime, wanted to flag that I added the timestep variable in the init as part of #77 to help with performance improvement. You just need to initialize the model with argument of time_steps equal to the number of timesteps you plan to run the model for (an integer).

[imscw95]

Ah thank you that solves the issue that I created for myself - setting that equal to 1 and trying to run multiple increment timesteps.

[imscw95]

Corrected all the syntax issues with the np.select. When I comment out the updateable_static_variable assignment and assert lines, all the tests pass, so the error is somewhere with that. It looks like updateable static variables are just supposed to be a variable name or list of variable names that get moved to the state variable list. Kelsey had it set up so that the updateable static var name = an existing static variable ('a0') - so you'd think that'd remove it from the static var list and put it on another list. I tried with a var name that didn't exist in the static vars and that didn't work either. Kinda confused by the error, but don't think updateable static variables are an essential part of the model, since it seems like we have different ways to handle variables that we know will be updated. Will plan to check in on it next week, but wondering what you both think.

[sjordan29]

Hi @kewalak, this looks great overall -- thanks for all the work you put into this. A lot of new variables and processes! Key things we should either address here or as part of future PRs:

• Some items on formatting / style:
  • Recommend using flake8 (flake8 \path\to\file\filename.py on any .py files to help identify stylistic updates to code to help increase general consistency and readability
  • Variable naming: tagged you in this question @aufdenkampe, but thinking we should try to more consistently name variables and processes with lower cases and underscores (see pep8). I think this could be a separate issue / MR though.
• Processes that include a time component: we need to think through how this is going to get handled if we have variable (user-defined) model timesteps. Tagged you in some of these comments @aufdenkampe -- is this a concern?
• Fix to test_6_nsm1_timestep: the updateable static variable provided (a10) is not a static variable identified by the model. We either need to add it if it should be in the model, or use a different static variable.
• I'd appreciate review from others on the test_7_nsm_algae_calculations.py since I don't feel super familiar with the process we're using to test these calculations.

[imscw95]

@sjordan29 It looks like a lot of changes in this PR came from me making edits in the processes files for the different state variables. I used those state variable-specific processes.py files to populate the nsm1\processes.py file, but have since been updating only that main processes.py file as I've tried to run the tests that @kewalak created, and have pushed commits to this branch mid-PR as I've found the syntax fixes that allow the tests to run. I apologize if that's not good coding etiquette as I think that's made things confusing. My main point is, I think a lot of the issues you point out in the sub-state variable processes.py files are fixed in the nsm1\processes.py file.

[sjordan29]

Thanks for pointing that out @imscw95- I'll go through and resolve all comments that appear to have been fixed!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.61%. Comparing base (a3bcb3c) to head (82a7ba9).
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #78       +/-   ##
===========================================
+ Coverage   42.86%   80.61%   +37.74%     
===========================================
  Files          24       27        +3     
  Lines        1185     1842      +657     
===========================================
+ Hits          508     1485      +977     
+ Misses        677      357      -320     

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

[sjordan29]

All tests pass! Everything has either been addressed or moved to new issues. @imscw95, do you want to do any additional review or shall we merge this to main?

[imscw95]

Amazing. New further reviews from me.