Repository navigation
Conversation
is_it_time_to_export compared the current time to each requested export time with np.isclose(atol=0, rtol=1e-5), and the post_processing loops never passed anything else. The window is relative to the current time, so at t = 0.9e8 it is 900 s wide: the export in the issue's example fired at t = 89999991, nine seconds early, and the matched time was consumed so nothing was written at 0.9e8 itself. Add export_time_atol (default 0) and export_time_rtol (default 1e-5) to F.Settings and pass them from the three post_processing loops. Defaults are unchanged, so existing runs behave as before. Negative values raise a ValueError, as Stepsize.milestone_tolerance does, since a negative tolerance would silently disable every timed export. The loops only read the settings when an export actually has times, so problems built without Settings (as some unit tests do) keep working. Fixes festim-dev#1074
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1290 +/- ##
==========================================
- Coverage 96.14% 96.12% -0.03%
==========================================
Files 57 57
Lines 5165 5183 +18
==========================================
+ Hits 4966 4982 +16
- Misses 199 201 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RemDelaporteMathurin
left a comment
RemDelaporteMathurin
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @steps-re for this! This looks good to me, I had a couple of comments
| * ``final_time``: the final time of a transient simulation | ||
| * ``stepsize``: the stepsize of a transient simulation, as a :class:`festim.Stepsize` or a number (see :ref:`Stepsize <stepsize_guide>`) | ||
| * ``element_degree``: the degree of the finite elements used for the concentrations (default: 1) | ||
| * ``export_time_atol`` and ``export_time_rtol``: the absolute and relative tolerances used to decide whether the current time matches one of the ``times`` requested by an export (default: ``0`` and ``1e-5``, see :ref:`Exporting at chosen times`) |
Collaborator
There was a problem hiding this comment.
Broader discussion we're having with @jhdark is this Settings docs page since this is basically a duplicate of the API-reference... probably not needed!
Comment on lines
+22
to
+29
| export_time_atol (float, optional): Absolute tolerance used to decide | ||
| whether the current time matches one of the ``times`` requested by | ||
| an export. Defaults to 0. | ||
| export_time_rtol (float, optional): Relative tolerance (with respect to | ||
| the current time) used to decide whether the current time matches | ||
| one of the ``times`` requested by an export. At large times the | ||
| relative tolerance alone can be very wide, and ``export_time_atol`` | ||
| should be used instead. Defaults to 1e-5. |
| my_model.show_progress_bar = False | ||
| my_model.run() | ||
|
|
||
| assert my_model.exports[0].t == [export_time] |
Collaborator
There was a problem hiding this comment.
Curiousity question as I'm not entirely sure: when we write an export, what time to we add to .t: is it the "real time" (eg. 0.9e8 +- epsilon) or the "target time" (here exactly 0.9e8)?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary
Adds
export_time_atol(default0) andexport_time_rtol(default1e-5) toF.Settings, and passes them tois_it_time_to_exportfrom the three post-processing loops. Defaults are unchanged, so existing runs behave as before.Related Issues
Fixes #1074
Motivation and Context
is_it_time_to_exportmatches withnp.isclose(time, current_time, atol=0, rtol=1e-5), so the window grows with time. At t = 0.9e8 it is 900 s wide. In the issue's example the export fires at t = 89999991, nine seconds early, and since the matched time is consumed, nothing is written at 0.9e8 itself.This follows the solution proposed in the issue: one pair of tolerances on
Settings, used by all exports. Negative values raise aValueError, asStepsize.milestone_tolerancedoes, since a negative tolerance would silently disable every timed export.The loops now check
getattr(export, "times", None) is not Nonebefore reading the settings, so problems built withoutSettings(as some unit tests do) keep working. Behaviour is otherwise identical:is_it_time_to_exportalready returnedTruefortimes=None.Type of Change
Testing
pytest)test_profile_export_time_at_large_timeis the issue's example withexport_time_atol=1e-6, export_time_rtol=0. It checks the export lands exactly on 0.9e8 and writes once.test_settings.pycovers the defaults, storing the values, and theValueErrorfor negatives.Against current
mainwithout the change, 5 of the new tests fail (4 intest_settings.py, plus the profile test). With it, all pass.Full suite on dolfinx 0.11.0 (macOS arm64), rebased on current
main: 1265 passed, 5 skipped, 0 failed.ruff format --checkandruff checkare clean on the touched files.Code Quality Checklist
ruff format .)ruff check .) (on the touched files)Documentation
userguide/settings.rst, and a paragraph under "Exporting at chosen times" inexport_post_processing.rstwith the large-time example.Additional Notes
With an adaptive stepsize, the solver also has to land on the requested time, so the docs point to
Stepsize.milestone_tolerancetoo. I left the defaults alone to avoid changing existing outputs. Happy to switch the default to an absolute tolerance if you'd rather.AI disclosure: I used an AI coding assistant (Claude) to help write and test this. I reviewed the change and ran the tests before and after the fix myself.