Split vertical fluxes on basin #1300
Conversation
|
@visr let me know whether you think this is going in the right direction. The vertical fluxes get integrated now but they are not yet saved. Something about this also feels a bit silly, since this does an approximation of the integral while we could easily obtain the exact value. |
|
In fact, for forward fill we don't need |
SouthEndMusic
force-pushed
the
split_vertical_fluxes
branch
from
f20390f to
560f695
Compare
|
Left to do:
|
|
Maybe #1084 can also be picked up in this PR, just expose Edit: it might not be that easy. We might need yet another copy of the vertical fluxes for the same reason we need |
| end | ||
|
|
||
| # Forget about vertical fluxes before basin update | ||
| copyto!(vertical_flux_prev, vertical_flux) |
There was a problem hiding this comment.
To be most accurate, maybe an exception should be made for vertical fluxes which are not set by the Basin \ time table.
There was a problem hiding this comment.
Typically only some of the vertical fluxes will be in Basin \ time. Could you make an issue about this? I don't fully oversee the error introduced in this case.
| end | ||
|
|
||
| # Forget about vertical fluxes before basin update | ||
| copyto!(vertical_flux_prev, vertical_flux) |
There was a problem hiding this comment.
Typically only some of the vertical fluxes will be in Basin \ time. Could you make an issue about this? I don't fully oversee the error introduced in this case.
This was missed from #1300. We don't store vertical flows anymore. Doing this was the only reason for `formulate_flows!` for Terminal nodes. What I don't fully understand is why tests did not fail on this. Any test model with a Terminal should have failed since the 3-arg `add_flow!(graph, id, -q)` it called was removed in #1300. This PR also removes the 4-arg version that is no longer used.
Fixes #661.