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

Added subroutine to set absolute tolerance of stream power law #44

Conversation

sebastianwolf
Copy link
Contributor

The current implementation of tol=1.d-4*(maxval(abs(h))+1.d0) can give high tolerance values for instance when fastscape's sealevel is at elevation of 600 km and the maximum elevation is 605 km. This can easily happen when coupling fastscape to a thermo-mechanical model that has the origin at the lower model boundary. Setting the default tolerance as done here should be re-considered.

…ded). The current implementation of tol=1.d-4*(maxval(abs(h))+1.d0) can give high tolerance values for instance when fastscape's sealevel is at elevation of 600 km. This can easily happen when coupling fastscape to a thermo-mechanical model. Setting the default tolerance as done here should be re-considered.
@benbovy
Copy link
Member

benbovy commented Apr 23, 2021

Thanks @sebastianwolf, that makes a lot of sense. This is also related to #40.

Maybe we could take this opportunity to implement a slightly more general and "standard" solution?

I'd suggest:

  • expose three parameters for the SPL solver in FastScape_ctx:

    • spl_atol: absolute tolerance
    • spl_rtol: relative tolerance
    • spl_max_iter: max. number of Gauss iterations
  • update the FastScape_Set_Tolerance_SPL subroutine that you've added in FastScape_api so that we can also set the other tolerance parameters (for more consistency, we might also rename this subroutine, e.g., FastScape_Set_SPL_Solver_Parameters).

  • Revisit the expression of SPL's convergence criteria so that it is based on both convergence parameters

Currently, the solution error is calculated using the RMSE from elevation taken at the times t and t+dt (i.e., hp and h, respectively). However I wonder if it wouldn't be more convenient to use absolute differences here, as it would be easier to deal directly with the elevation units (instead of squared units) for the absolute tolerance parameter.

We could use a common convergence criteria like:

all(abs(h-hp) < spl_rtol * abs(hp) + spl_atol)

What do you think? Any thoughts @jeanbraun ?

@sebastianwolf
Copy link
Contributor Author

@benbovy this sounds all good to me. I have no substantially qualified opinion on how your convergence criteria should look like, as long as it is modifiable :).

@benbovy
Copy link
Member

benbovy commented Dec 2, 2022

Hi @sebastianwolf!

Finally, both relative and absolute tolerance (as well as a max. iteration number) have been added in Jean's PR #51. So I guess we can close this one?

Thank you again for your contribution, and sorry for the tremendous wait!! Although we are still trying to maintain this library, the main development efforts are currently put in https://github.com/fastscape-lem/fastscapelib.

@sebastianwolf
Copy link
Contributor Author

That sounds good, no problem. Yes, this can be closed.

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