Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ptl add adaptive timestep #25

Merged
merged 13 commits into from
Jun 26, 2024
Merged

Ptl add adaptive timestep #25

merged 13 commits into from
Jun 26, 2024

Conversation

peterlafollette
Copy link
Contributor

@peterlafollette peterlafollette commented May 9, 2024

Adaptive time step was added and tested for stability.

Additions

-The model can now run with an adaptive time step. The adaptive time step can be turned on or off in the config file. If it is not specified in the config file, it defaults to false. The time step is equal to the forcing resolution when precip and ponded head are 0, which saves a lot of runtime in arid areas where large periods of time will not have precip. The time step is equal to two times a user specified minimum time step when the precip is less than or equal to 1 cm/h and there is no ponded head. The time step is equal to a user specified minimum time step (which is just taken from "timestep" in the config file when "adaptive_timestep" is set to true) when ponded head is greater than 0, or when precip is greater than 1 cm/h. The time step can not be greater than the forcing data resolution, and this is enforced.

Removals

Changes

-Updated README in configs to include adaptive_timestep.

-The giuh convolution integral is now computed at the temporal resolution of the forcing data rather than at the resolution of the sub timestep. The giuh ordinates are still resampled, but to the resolution of the forcing data rather than to the internal time step, which effectively means that the giuh ordinates are not resampled if the forcing data resolution is 1 hour.

-Discovered a bug where theta is very close to theta_r, and convergence might be possible, but the function lgar_theta_mass_balance doesn't converge after a few minutes. The bug comes from the fact that the condition (fabs(delta_mass - delta_mass_prev) < 1E-15) doesn't trigger; the change in mass is indeed small, but it is 1E-11 rather than 1E-15 and changes very slowly, just because of the sensitivity of the theta-psi relationship for some parameter sets in particular. Rather than changing the tolerance to be less strict, I added a new check where the loop will break for extremely large psi values, which have limited physical meaning in the first place.

-Changed the psi value at which calc_Se_from_h returns 1.0 to be 1E-10 rather than 1E-1, making the function accurate for small psi values. There are some cases where we need Se values that are very close to but slightly less than 1, for psi values that are less than 1E-1.

Testing

  1. Stability testing with 40 k parameter sets successful (adaptive and fixed time steps).
  2. Phillipsburg, Bushland, synthetic, and unit tests give expected outputs.
  3. Integrated tests on GitHub.
  4. Tested in ngen, including 3 layer tests, 1 layer tests, and running multiple catchments from the same realization file, including a mix of adaptive and fixed time step config files.

Screenshots

Notes

Todos

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Visually tested in supported browsers and devices (see checklist below 👇)
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Target Environment support

  • Windows
  • Linux
  • Browser

Accessibility

  • Keyboard friendly
  • Screen reader friendly

Other

  • Is useable without CSS
  • Is useable without JS
  • Flexible from small to large screens
  • No linting errors or warnings
  • JavaScript tests are passing

@peterlafollette peterlafollette requested a review from ajkhattak May 9, 2024 17:48
configs/README.md Outdated Show resolved Hide resolved
giuh/giuh.c Outdated Show resolved Hide resolved
include/all.hxx Outdated Show resolved Hide resolved
src/bmi_lgar.cxx Outdated Show resolved Hide resolved
src/bmi_lgar.cxx Outdated Show resolved Hide resolved
src/bmi_lgar.cxx Show resolved Hide resolved
src/lgar.cxx Outdated Show resolved Hide resolved
src/lgar.cxx Outdated Show resolved Hide resolved
Peter La Follette and others added 12 commits June 11, 2024 12:49
…as the smallest adaptive time step for the rest of the model. If the adaptive time step is off, the giuh time step will be the same as the timestep input in the config file.
…ive time step strategy does not alter the function in giuh.c because this function is used in multiple models. So, the adaptive time step method has been altered such that the giuh is computed at the hourly time step rather than at the sub timestep. This allows for the giuh fxn to be unaltered, and has the added bonus of not reqiring resampling for the giuh based on time step.
… removing hardcoding for internal time steps and replacing with user specified minimum time step, and establishing a maximum time step equal to the forcing resolution
@peterlafollette peterlafollette force-pushed the PTL_add_adaptive_timestep branch from 0f480b9 to 38e8037 Compare June 11, 2024 19:55
Copy link
Collaborator

@ajkhattak ajkhattak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

@ajkhattak ajkhattak merged commit 01dc874 into master Jun 26, 2024
8 checks passed
hellkite500 pushed a commit to hellkite500/LGAR-C that referenced this pull request Jul 10, 2024
* adding the capability for an adaptive time step. If not provided in the config file, it defaults to false.

* updating readme

* updating readme

* updating giuh fxn with adaptive time step

* giuh now uses a fixed time step that is hardcoded to be the the same as the smallest adaptive time step for the rest of the model. If the adaptive time step is off, the giuh time step will be the same as the timestep input in the config file.

* updating readme in configs

* cleaned up code that was used for debugging

* updating Phillipsburg config file

* cleaned up code that was used for debugging

* After chatting with Ahmad, we determined that it is best if the adaptive time step strategy does not alter the function in giuh.c because this function is used in multiple models. So, the adaptive time step method has been altered such that the giuh is computed at the hourly time step rather than at the sub timestep. This allows for the giuh fxn to be unaltered, and has the added bonus of not reqiring resampling for the giuh based on time step.

* updating in response to comments, mostly focused on removing hardcoding from min and max time step values

* more changes in response to comments, including formatting of giuh.c, removing hardcoding for internal time steps and replacing with user specified minimum time step, and establishing a maximum time step equal to the forcing resolution

* removing code that was commented out

---------

Co-authored-by: Peter La Follette <[email protected]>
Co-authored-by: Peter La Follette <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants