SIP147 Retool events as fluxes - #148
Conversation
dlebauer
left a comment
dlebauer
left a comment
There was a problem hiding this comment.
Looks nice - I like the change to flux-based event handler and the general direction of ordering flux calcs then pool updates! And thanks for catching the bug in how immedEvap was handled.
- Since I only see changes to the irrig flux, I assume that this PR doesn't actually change the values or implied units written out, it only changes how they are internally calculated. Correct?
- see two inline comments:
- check for climLen <=0?
- change comment from (update pools --> update fluxes).
| trackers.yearlyLitter += fluxes.leafLitter; | ||
| } | ||
|
|
||
| void resetEventFluxes(void) { |
| // should. | ||
| const int climYear = climate->year; | ||
| const int climDay = climate->day; | ||
| const double climLen = climate->length; |
There was a problem hiding this comment.
Not really related to this PR, except that this is so often a demoninator: is there or should there be a check that climLen > 0?
There was a problem hiding this comment.
Yes, definitely - never assume your data is good!
Co-authored-by: David LeBauer <dlebauer@gmail.com>
@dlebauer : I think this IS changing the implied units, from pool amounts (eg g C/m^2) to fluxes (eg g C/m^2/day) |
What I meant was probably unclear but it was just that when units were previously mass/area, the daily time step implicitly added the /d even if this wasn't explicit. Which is why the outputs don't change. |
2 participants
Fixes #147
This PR:
Notes:
sipnet.outhas not changed in the smoke tests (events.outhas been updated, though, with expected changes). Second commit updates tests (and found some issues, hurray tests!). Second commit has the green check :-)fluxes.immedEvap) correctly; test updated to reflect thissoilDegradationinto separate fluxes and pool updates, to keep this PR smaller (and, that code change should not be a functionality change, so best to leave it out)