Fix: prevent duplicate exports by popping matched time in is_it_time_…#1071
Fix: prevent duplicate exports by popping matched time in is_it_time_…#1071AdriaLlealS wants to merge 1 commit intofestim-dev:fenicsxfrom
Conversation
…to_export This updates `is_it_time_to_export` so that when a timestep matches a desired export time (within the `np.isclose` tolerances), the corresponding entry in `times` is popped. Previously, multiple consecutive timesteps within the tolerance window could all trigger `True`, leading to repeated exports for the same target time, especially at high simulation times where the relative tolerance (rtol) allows larger absolute differences. With this change: - only the first matching timestep triggers an export - the matched time is removed, preventing any subsequent exports for it This ensures a 1-to-1 mapping between requested and actual exports, provided the simulation reaches timesteps sufficiently close to the target times.
| for i, time in enumerate(times): | ||
| if np.isclose(time, current_time, atol=atol, rtol=rtol): | ||
| times.pop(i) # consume the time so it is not exported again |
There was a problem hiding this comment.
Would it be safer to make a copy of times elsewhere in order to avoid destroying user inputs?
|
Hi @AdriaLlealS thanks for this! Maybe it would be helpful to make an example reproducing the issue? That way we can check that this fix is working. I think we should make a copy of the |
Absolutely, I'll create a copy of 'times' to solve this. About the example, should I post it in the discourse group and then somehow link it here in the PR or how would you suggest doing it? |
That's ok just post it here |
2 participants
Description
Summary
This PR updates
is_it_time_to_exportso that when a timestep matches a desired export time (within thenp.isclosetolerances), the corresponding entry intimesis popped. This prevents repeated exports for the same target time.Related Issues
Motivation and Context
Previously, multiple consecutive timesteps inside the tolerance window could all return
True, causing repeated exports for a single requested export time, especially at high simulation times where the relative tolerance (rtol) allows larger absolute differences. This change ensures a 1‑to‑1 mapping between requested and actual exports, provided the simulation reaches timesteps sufficiently close to the target times.Type of Change
Testing
pytest)Code Quality Checklist
ruff format .)ruff check .)Documentation
Breaking Changes
Screenshots/Examples
Additional Notes
Even though duplicates are now removed, very large simulation times can still cause the matched timestep to be several seconds away from the desired export time. Exposing tolerances (e.g. reducing
rtol) cannot fully solve this issue: tighteningrtolonly delays it and may prevent matching early in the simulation, while relying on a smallatolmay fail when the timestep becomes larger near an export event.If precise export timings are required, one way to mitigate this limitation is to use a small
atoland ensure that the desired export times are included in the solver’s milestones so that the simulation visits them closely enough.