Skip to content

Conversation

@xaviernogueira
Copy link
Contributor

@xaviernogueira xaviernogueira commented Nov 8, 2023

Closes #49

Changes:

  • I looped back to fix the logic for TSM's ri_function using two-step logic following a misunderstanding in my last PR @aufdenkampe
  • I created the feature discussed in Issue Create "convert static to state" function #49 where one can init a Model with an optional list of static variable names, updateable_static_variables that allow them to be updated between timesteps (essentially gives them a time dimension). See example below
  • I used pyright (a handy command line tool) to fix a few unaligned type hints
  • I added test coverage for the new static variable update feature -> all tests are passing

@sjordan29 @jrutyna You can now proceed as discussed.

For example, imagine a model with a 1 state variable called "state_variable_1" and two static variables "static_1" and "static_2".
Despite the static variables not being changed by the model itself (hence why they are called static), one might want to alter "static_1" between running timesteps via another model like Clearwater-Riverine. To do so you simply init the model as shown below:

import clearwater_modules.base as base

my_model_instance: base.Model = Model(
     initial_state_values={'state_variable_1': 1.0},
     updateable_static_variables=['static_1']
)

then when running a timestep you can use the update_state_values argument, which takes a dictionary with variable names as keys, and xr.DataArrays as values. For now, this requires data arrays, but I am going to open a new issue to support just passing in a number, and having that number broadcast to the shape of our xr.Dataset, see #52 .

@xaviernogueira xaviernogueira added enhancement New feature or request refactor-core For core functionality, not model specifc labels Nov 8, 2023
@xaviernogueira xaviernogueira linked an issue Nov 8, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #51 (c38c6b6) into main (5772cf2) will increase coverage by 1.54%.
Report is 1 commits behind head on main.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
+ Coverage   36.18%   37.72%   +1.54%     
==========================================
  Files          33       33              
  Lines        1133     1153      +20     
==========================================
+ Hits          410      435      +25     
+ Misses        723      718       -5     
Files Coverage Δ
src/clearwater_modules/tsm/processes.py 71.42% <100.00%> (+0.37%) ⬆️
src/clearwater_modules/utils.py 58.82% <0.00%> (ø)
src/clearwater_modules/base.py 87.29% <85.71%> (+4.57%) ⬆️

xaviernogueira and others added 2 commits November 8, 2023 12:30
But @xaviernogueira, please confirm that the `default=` argument to `np.select()` can point to an array. The say it needs to be a scalar, but perhaps this happens when you use a ufunc anyways.
@aufdenkampe
Copy link
Member

@xaviernogueira, thanks for moving this forward! I only did a quick scan, but it generally looks good!

@xaviernogueira xaviernogueira merged commit f88338b into main Nov 8, 2023
@xaviernogueira xaviernogueira deleted the 49-create-convert-static-to-state-function branch November 8, 2023 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request refactor-core For core functionality, not model specifc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create "convert static to state" function

3 participants