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

Changed eps in timestepping #254

Merged
merged 2 commits into from
Feb 7, 2024
Merged

Conversation

AshGannon
Copy link
Collaborator

We use an epsilon to get the "expected" behavior when the deposition time and the time match should match exactly but don't because of floating point accuracy. I changed eps to be computed using the deposition start time and error between the time step and activation time.

@AshGannon AshGannon marked this pull request as draft February 1, 2024 18:17
application/adamantine.hh Outdated Show resolved Hide resolved
application/adamantine.hh Show resolved Hide resolved
application/adamantine.hh Outdated Show resolved Hide resolved
application/adamantine.hh Outdated Show resolved Hide resolved
application/adamantine.hh Outdated Show resolved Hide resolved
@AshGannon
Copy link
Collaborator Author

I only ended up changing the denominator of eps.

@AshGannon AshGannon marked this pull request as ready for review February 2, 2024 19:49
@stvdwtt
Copy link
Collaborator

stvdwtt commented Feb 2, 2024

@AshGannon do you have a sense for when the epsilon is too small and when it isn't? Are we safe hard-coding it like you have, or should it depend on something problem specific?

@Rombur
Copy link
Member

Rombur commented Feb 2, 2024

The tolerance already depends on the problem through the time step. It's obviously not ideal to have a "magic" number in the code but when dealing with round-off you usually don't have a choice.

The PR is not passing the CI but that's because we need to merge #253 first.

@AshGannon
Copy link
Collaborator Author

AshGannon commented Feb 2, 2024

@AshGannon do you have a sense for when the epsilon is too small and when it isn't? Are we safe hard-coding it like you have, or should it depend on something problem specific?

Bruno's answer is better.

Bruno mentioned to me that he originally chose 1e12 because you can expect an accuracy of about 12 digits with double precision. He also uses this eps for power and time in deposition_along_scan_path. In my hourglass simulation, the dwell time is set to 0, so my deposition_time.begin() happened to be 1e-12. The deposition_time was also not set. Since double eps = (*deposition_times.begin())/abs(activation_time_end - time); worked in this example, double eps = 1e-12/(time - time_old) might work generally. It's almost the same as dt/1e12, but you accumulate error in dt as time goes on.

@stvdwtt
Copy link
Collaborator

stvdwtt commented Feb 7, 2024

Retest this please

@AshGannon
Copy link
Collaborator Author

@stvdwtt I'm not sure what you mean. Retest the 1e-10? Or the dt/time-time_old? I ran the latter over the weekend, the mesh fully activated and the results were very close at the times I checked (just spot checked). Are you seeing something odd with the 1e-10?

@stvdwtt
Copy link
Collaborator

stvdwtt commented Feb 7, 2024

@AshGannon Sorry! That wasn't directed at you -- that's the command for the continuous integration tests to run again. I just merged in Bruno's PR and my understanding is that the tests in this PR should pass now. Assuming they do, we'll merge this.

@stvdwtt stvdwtt merged commit 38a7cf2 into adamantine-sim:master Feb 7, 2024
1 check passed
@AshGannon AshGannon deleted the changing_eps branch June 18, 2024 15: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.

None yet

3 participants