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

Simplify/Fix Adaptivity for Small Errors or no CFL #548

Open
wants to merge 38 commits into
base: develop
Choose a base branch
from

Conversation

Steven-Roberts
Copy link
Collaborator

The TINY parameter used in the controllers could cause an error overestimation, particularly for the first step.

Copy link
Collaborator

@drreynolds drreynolds left a comment

Choose a reason for hiding this comment

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

I am not expert on how robustly various architectures/compilers work with infinity. I'd like to see this PR pass all possible regression tests on all architectures before approving, but if infinity is handled correctly everywhere, then I would approve of these changes.

As another option, we could use the largest/smallest representable floating point numbers in the given precision to avoid having to do arithmetic with infinity.

@Steven-Roberts
Copy link
Collaborator Author

The behavior of pow, abs, min, max, and other functions for inf/nan is specified by the C standard, so I'm not too worried about that. I think a unit test where the error is 0 is warranted to double check. The standard only specifies that pow(0, 0) may cause a domain error (and doesn't even specify what the value could be), and pow(0, y) can cause floating point exceptions for certain y. I think I'll need to handle that more carefully in the controller.

@Steven-Roberts
Copy link
Collaborator Author

After considering the edge cases around pow(0,0), i.e., when there's no local error and a controller exponent is 0, it turns out to be benign. The C standard specifies pow(x,0)=1 for all x, and there's no undefined behavior. The test I added seems to support this for some limited cases.

@Steven-Roberts Steven-Roberts marked this pull request as ready for review September 10, 2024 23:08
Copy link
Member

@gardner48 gardner48 left a comment

Choose a reason for hiding this comment

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

The recent updates look good, once the CI is passing and the TODO is addressed I think this will be good to merge

@gardner48 gardner48 added this to the SUNDIALS After Next milestone Dec 5, 2024
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.

3 participants