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

Incorporate SmoothInterpolation #1543

Merged
merged 3 commits into from
Jun 17, 2024
Merged

Incorporate SmoothInterpolation #1543

merged 3 commits into from
Jun 17, 2024

Conversation

SouthEndMusic
Copy link
Collaborator

@SouthEndMusic SouthEndMusic commented Jun 10, 2024

Fixes #1482
Fixes #566

  • If we actually use this, it would probably make sense to give you admin rights on SouthEndMusic/SmoothInterpolation.jl (github.com)
  • Whether it is actually useful to use smoothing of the linear interpolations remains to be seen. Currently the implementation uses a sampled SmoothedLinearInterpolation which is a LinearInterpolation. The only new interpolation type used in the simulation is LinearInterpolationIntInv, which is a much more concise implementation of what was in get_area_and_level
  • One takeaway of this PR regardless of the use of SmoothedLinearInterpolation is that we can get rid of quite some utility code, also partly because of making more use of derivatives and integrals via DataInterpolations.jl

@SouthEndMusic
Copy link
Collaborator Author

I smelled something fishy when I found out that on this branch the AGV model takes 3-4x longer than on main. No tests are failing, and noting remarkable shows up in the profile. So I investigated a bit, and I found that the profile for certain basins behaves weird in the old code:

weird_plot

I haven't figured out what triggers this yet, but apparently it doesn't happen in our test models, at least not where tests depend on it. I'm pretty sure the new code (in SmoothInterpolation) is more correct and robust because it is tested more elaborately.

Copy link
Member

@visr visr left a comment

Choose a reason for hiding this comment

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

Great that we can get rid of so much utility code, and have a more solid basis to work on.

@visr visr merged commit 4e7ea0e into main Jun 17, 2024
25 checks passed
@visr visr deleted the use_smoothinterpolation branch June 17, 2024 14:09
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.

Use FindFirstFunctions for lookups in sorted lists Smoother interpolation of basin profile and Q(h) relation
2 participants