feat: Make model runs end at the exact time specified

[elbeejay]

Jules tackled #115. I think these changes would close #115

---

This change modifies the model run loop to ensure that the simulation ends at the exact time you specified, even if the total run time is not a multiple of the model's timestep dt.

The _SerialJob and _ParallelJob classes in pyDeltaRCM/preprocessor.py have been updated to:

1. Calculate the number of full timesteps (whole) and the remainder time (rem) required to reach the specified end time.
2. Run the model for whole full timesteps.
3. If there is a rem > 0, set the model's dt to the remainder and run one final timestep.

A test case has been added to tests/integration/test_timing_triggers.py to verify this new behavior.

[elbeejay]

Looked like the tests passed on the feature branch. Changes are minor and look fine to me, including the new test.

[amoodie]

Looks like a good start, but I think there are a few things to consider before merging.

The sediment routers receive the timestep during instantiation, and so I think init_sediment_routers() definitely needs to be called again if the timestep is changed.

But that dt in the sediment routers actually only affects the calculation of qs, which isn't really used for much and is just exported for other analysis.

More importantly, the timestep is initially determined as a function of input sediment volume. The volume of sediment per parcel is determined indirectly from this input volume as well https://github.com/DeltaRCM/pyDeltaRCM/blob/8a4ab350d9def2a1e914e7130f3dab9ce8c9a467/pyDeltaRCM/init_tools.py#L441. I think that if that parcel volume is not changed as well, then the sediment routing during the last smaller timestep will have the same sediment load as a full timestep. So, at least Vp_sed should be updated as well, and maybe some other variables too; these changes should be made before reinitializing the sediment routers.

There may be other places the timestep is used during initialization, but these are just the two that come to mind right away.

[amoodie]

Also: I guess this may highlight that we don't actually have a test that tests for mass conservation? Like: deposited volume == input_volume * elapsed_time. That seems like an important thing we should be testing.

[elbeejay]

Looks like a good start, but I think there are a few things to consider before merging.

The sediment routers receive the timestep during instantiation, and so I think init_sediment_routers() definitely needs to be called again if the timestep is changed.

But that dt in the sediment routers actually only affects the calculation of qs, which isn't really used for much and is just exported for other analysis.

More importantly, the timestep is initially determined as a function of input sediment volume. The volume of sediment per parcel is determined indirectly from this input volume as well https://github.com/DeltaRCM/pyDeltaRCM/blob/8a4ab350d9def2a1e914e7130f3dab9ce8c9a467/pyDeltaRCM/init_tools.py#L441. I think that if that parcel volume is not changed as well, then the sediment routing during the last smaller timestep will have the same sediment load as a full timestep. So, at least Vp_sed should be updated as well, and maybe some other variables too; these changes should be made before reinitializing the sediment routers.

There may be other places the timestep is used during initialization, but these are just the two that come to mind right away.

Good points - I need to review the latest changes (which supposedly adjust sea level rise and subsidence in addition to sediment supply).

[codecov]

Codecov Report

❌ Patch coverage is 30.00000% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.50%. Comparing base (8a4ab35) to head (37eb27d).
⚠️ Report is 10 commits behind head on develop.

Files with missing lines	Patch %	Lines
pyDeltaRCM/preprocessor.py	30.00%	21 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #291      +/-   ##
===========================================
- Coverage    79.00%   78.50%   -0.50%     
===========================================
  Files           12       12              
  Lines         2643     2671      +28     
===========================================
+ Hits          2088     2097       +9     
- Misses         555      574      +19     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[elbeejay]

Also: I guess this may highlight that we don't actually have a test that tests for mass conservation? Like: deposited volume == input_volume * elapsed_time. That seems like an important thing we should be testing.

My understanding is that the model is explicitly not mass-conservative because we stop sediment routing if/when the parcel hits the "stepmax" threshold (or the edge boundary condition), even if that parcel still contains sediment.

[amoodie]

The model is supposed to be mass conservative of sediment, at least generally and in it's overall formulation. More specifically, the model is actually volume conservative, since there is no porosity term. The model is explicitly not mass conversative of water, though, and explicitly not conservative of the difference between sand and mud (but rather just total sediment volume).

Volume leaving the domain is okay, so long as we know about it. I forget what happens when stepmax is reached, but I thought that material is just dropped in place, not discarded. So in either case, mass (volume) should be conserved (not creating new mass or losing track of mass).

We could check that by differencing topography after 10 time steps from the initial topography, and comparing that to the volume input times elapsed time. They should be the same. The exception is likely to be the inlet cells, where a boundary condition on elevation is applied, violating conservation there.

We could also do this by loading up the checkpoint run, just to see how close things are after a long period of time.

You don't need to do these for this PR/issue, just talking out load.

[amoodie]

changes

this looks good now, I like that the extra timestep is handled with a preprocessor method and is very readable. I can't think of other parts of the model that would need to be accounted for in changing the timestep.

tests

• I think we don't want/need this AI-added "run_rest.py" file?
• Should we also add a test of the preprocessor handling this extra timestep itself ? The test looks like it manually does what the method does, so it isn't actually testing the software, just the idea of the software...

[elbeejay]

I adjusted the test to use the _SerialJob class directly so we can at least compare the end-time when running for 2.5 time steps against the expected end-time. Not the most perfect test, but better than what was previously proposed I think.

[amoodie]

looks good, thanks for making those changes 👍