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

Change to RootSolvers tolerance #141

Closed
wants to merge 2 commits into from
Closed

Conversation

charleskawczynski
Copy link
Member

This is an alternative to #140.

Pros: the interface is likely more stable (assuming we'll continue with RootSolvers), since this gives us a lot of flexibility in specifying the tolerance.
Cons: users must depend on RootSolvers if they want to use custom tolerances

Similar to #140, in this PR we don't directly have the temperature_tol, and need some estimate for handling how we break out into freezing edge cases:

freezing_temperature_tol(tol::RS.ResidualTolerance, ::Type{FT}) where {FT} =
    tol.tol
freezing_temperature_tol(tol::RS.AbstractTolerance, ::Type{FT}) where {FT} =
    FT(0.1)

# then, when we need temperature_tol:
temperature_tol = freezing_temperature_tol(tol, FT)

Copy link
Contributor

@tapios tapios 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 fine. But I'm not sure we need this flexibility.

An added advantage of just using solution tolerances likely is that with those, we do not need to catch the non-differentiable region around freezing. We just needed to do that if we specify tolerances on energy variables, which jump at the phase transition. But temperature is continuous, so with solution tolerances everything probably works without the edge case catch.

e_int,
q_tot,
100,
RS.RelativeSolutionTolerance(FT(1e-6)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this not to be hardwired. E.g., use sqrt(eps) in place of 1e-6.

@@ -683,7 +691,7 @@ end
e_int,
q_tot,
100,
FT(1e-3),
RS.RelativeSolutionTolerance(FT(1e-6)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@charleskawczynski
Copy link
Member Author

Superseded by #140

@charleskawczynski charleskawczynski deleted the ck/general_tolerances branch November 11, 2022 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants