-
Notifications
You must be signed in to change notification settings - Fork 50
Added NaNReporter and HighMaReporter to LettuceCFD #248
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
base: master
Are you sure you want to change the base?
Added NaNReporter and HighMaReporter to LettuceCFD #248
Conversation
|
Hello @MaxBille! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-09-05 09:53:25 UTC |
shortened class definitions
adaptations of new reporter to native architecture, thx PhiSpel
pspelt is on fire... 🔥 👨💻
slightly more flexibility for step in which NaN fail occurs
PhiSpel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Could you check if we need to set non_native in BreakableSimulation? If so, can you rename to
BreakableSimulationNonNative? - Maybe rename example to
FailingTGVandObstacle.py?
- renamed FailingTGVandObstacle.py - removed hardcoded non-native execution from breakable simulation class. - set Obstacle flow to non-native
|
@PhiSpel would you mind, reviewing the changes I've made, regarding your change requests and recommendation? :) (if you have the time...) |
…ed super-class, cleaner output und functionaliy
PhiSpel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments and changes still.
A nice setup!
Please remove lettuce/ext/_reporter/nan_repoter.py (the comments still apply to lettuce/ext/_reporter/failure_reporter.py!)
Generally, please revisit the typing of inputs and outputs of the classes and functions :)
lettuce/ext/_reporter/nan_repoter.py
Outdated
| self.failed_iteration = None | ||
| super().__init__(interval) | ||
|
|
||
| def __call__(self, simulation: 'Simulation'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you import Simulation with no apparent issues, you can type it directly. Keep in mind, though: This must be a BreakableSimulation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MaxBille have you tried this?
lettuce/ext/_reporter/nan_repoter.py
Outdated
| f'details!') | ||
| # telling simulation to abort simulation by setting i too high | ||
| simulation.flow.i = int(simulation.flow.i + 1e10) | ||
| # TODO: maybe make this more robust with a failed-flag in simulation and not rely on flow.i to be high/low? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Since we're working with BreakableSimulation anyway, it can just get a flag lastIteration, such that any other Reporter (also VTKReporter!) can also check if the simulation failed and output one last time. This is also implemented in NATriuM.
This may have been my own TODO-comment, btw ... :) So, for now, you could just post ths into another issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MaxBille is this a won't-do, or something for a new issue?
cleanup and small corrections to comments, typing and code formating
| stencil=lt.D2Q9 | ||
| ) | ||
| nan_reporter = lt.NaNReporter(100, outdir="./data/nan_reporter", vtk=True) | ||
| simulation = lt.BreakableSimulation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the reporters only work with this specific Simulation class (i.e. BreakableSimulation)?
If not, I prefer to initialize this class within the example script (i.e. header).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MaxBille
Is there an update on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@McBs yes, it should be BreakableSimulation since we manipulate simulation.flow.i. If simulation this is not BreakableSimulation, it will continue running because call() contains a blind for-loop (
lettuce/lettuce/_simulation.py
Line 243 in d04e156
| for _ in range(num_steps): |
PhiSpel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing is still incomplete
Description
Closes #174
Closes #272
Adding two reporters, that check for signs of simulation crashes (NaN in f or Ma > 0.3 in u) and abort simulation with respective abort messages, optional final vtk-frame and (in case of HighMaReporter) optional output file with locations of the 100 highest velocity magnitudes for crash-troubleshooting and analysis.
NaNReporter:
HighMaReporter:
DISCLAIMER: originally written for a modified version of lettuceCFD, based on Version 0.2.3; Adaptation for new Lettuce Master pending...
Checklist