Skip to content

Conversation

@jeremyfirst22
Copy link
Contributor

Bugfix

  • The min_steady_timesteps database parameter was never used to exit upon converged Ca, since Ca_previous was never updated, causing ((fabs((Ca - Ca_previous) / Ca) < tolerance) to always be false. Added a break to exit once the steady point is achieved to avoid continuing to write steady point information until the EXIT_TIMESTEP is achieved.

bool isSteady = false;
if ((fabs((Ca - Ca_previous) / Ca) < tolerance &&
if ((fabs((Ca - Ca_previous) / analysis_interval / Ca) < tolerance &&
CURRENT_TIMESTEP > MIN_STEADY_TIMESTEPS))
Copy link
Collaborator

Choose a reason for hiding this comment

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

why add analysis_interval here? The previous logic is simply comparing the relative change in the capillary number. This change will alter the interpretation of tolerance

Copy link
Contributor Author

@jeremyfirst22 jeremyfirst22 Dec 1, 2025

Choose a reason for hiding this comment

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

@JamesEMcClure I added the analysis_interval here to make the tolerance relative to each simulation step, and independent of the number of steps between evaluating the capillary number.

i.e., if the simulation is not changing by greater than 1% per step, we consider the simulation converged. Not if the simulation is changing by 1% per analysis interval.

The idea was that you should be free to change the analysis_interval without having to adjust the tolerance in kind to achieve the same results.

I agree this changes the interpretation of tolerance, but since the Ca_previous was never updated anyway (i.e., tolerance was never used), I figured I was free to update the interpretation. But 1% per step is an extremely loose default. If you'd like, I will update the default to be consistent with the previous interpretation (1% / 1000 steps)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JamesEMcClure I went ahead and make this change, and updated the default tolerance to 1e-5 so that is consistent with the old interpretation of the default (i..e, 0.01 over the default 1000 steps).

Let me know if you have any other comments on this one.

@jeremyfirst22 jeremyfirst22 force-pushed the bugfix-min-steady-timesteps branch from 7761529 to 6b121eb Compare December 17, 2025 19:32
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