-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
more detailed debugging outputs, solver documentation, and high loglevel print improvements #1759
more detailed debugging outputs, solver documentation, and high loglevel print improvements #1759
Conversation
d0cf75a
to
117e41a
Compare
I realized that I hadn't been doing the proper thing with regards to the line search, and so for now I just reverted back to the original method. I added some documentation about the method as it was clearly explained in the Kee book. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1759 +/- ##
==========================================
- Coverage 73.23% 73.19% -0.04%
==========================================
Files 381 381
Lines 54343 54371 +28
Branches 9246 9248 +2
==========================================
+ Hits 39796 39799 +3
- Misses 11573 11593 +20
- Partials 2974 2979 +5 ☔ View full report in Codecov by Sentry. |
Based on the description of the Newton solver in the Kee book, there was a thing that didn't quite make sense to me in the damped newton solver where fbound was multiplied by the damping factor. I think that fbound just serves as the starting point for the correction vector length, but in the implementation it was always multiplying the damping factor by the fbound, which I think might have artificially reduced the damping factor lower than necessary. |
I may have gone overboard on the solver print statements. The logging outputs when loglevel was set to a high value were...quite perplexing in their current form. |
For anyone interested, I created a simple 2 equation, 1 point control model problem that I was using just to examine the behavior of the algorithm on a small-scale problem.
|
One strange thing that I notice is that if we add the temporal term of the continuity equation to the continuity residual, the standard non-two-point control solve() fails. This seems odd to me. The time component from the Kee book is just d(rho)/dt
|
Examples like this could be added under |
The time-dependent solver only supports terms that end up on the diagonal of the Jacobian (hence the name of the variable |
Ah yep. That's definitely it. 🤦♂️ |
If I take the standard solver, and call solve() with successively higher boundary mdot values, going from a starting value of 0.5 to around 7, and then I activate the two-point control, it has no problems with lowering the temperature control points down. Even an increment of 10 K converges without issues ( used to crash at an increment of 3K). This is all at high pressure (5.4e6 Pa). So maybe there's a possibility that the newton solver is seeing some sort of solution at a negative left boundary velocity for low boundary velocities. I'm not sure if there's anything that can be done about that. |
I may be doing something wrong with the Python interface. To set the value of the m_mdot variable at the left and right boundaries for the 1D counterflow diffusion flame, we use the In the script below, I have an initial solution at the 5.4e6 Pa, solved using the
|
How recently have you rebased? This may be an artifact of something that @speth fixed recently in #1740 (specifically #1629). Essentially, some updated data were overwritten by older data during serialization. |
I will try that. The issue seems to be that, even though I have specified that overwrite=True option, the backup_2.yaml file is not being overwritten. The large values were from a previous run that ran up past the extinction mdots and went to very high values of mdot during the loop. |
I spoke too soon on this. Examining the solution, the left boundary is still going negative, it just is somehow converging to a solution that has a negative u at the left boundary. Somehow that dF(u)/dLambda is getting flipped around. |
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.
Thanks, @wandadars. The expanded docstrings for MultiNewton
are very welcome, and I think the updates to how debug_sim1d.yaml
is updated are looking pretty good. Please do look at the rendered output, either by downloading the "docs" artifact (click on "Checks" on this page, then on the "Artifacts" dropdown in the top center of the page) or by building the Doxygen docs locally. There are a number of formatting issues which you'll easily see and be able to address.
For the changes to the logging, I'd suggest a bit more work, and taking a look at what these files contain at all log levels (1-5). We want to make sure that the lower log levels are still fairly concise. That may involve leaving a few things that are less clear at high log levels, where some of the data gets interleaved a bit. One thought that came to mind was for the "tabular" outputs, you could try putting some short, distinct prefix (say 2 characters) on those lines so you could at least tell what lines were coming from which part of the solver. No idea if this would end up being horribly ugly or actually helpful.
3e9ccf8
to
d67f272
Compare
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.
Minor suggestions.
d67f272
to
5f52e68
Compare
Ok, I think I got the line lengths down a bit. I made a new compressions, such as just saying "log(ss)", because we are really just looking at the change in that value, and not the magnitude for the most part. We compare to other values, and so the exact type of logarithm didn't seem critical. I have some long lines, but I'm not sure what the best way to split them would be so as to not make reading the code overly complicated. I might be missing some fmt-fu that would help to better handle the output positioning information. I've reviewed the different levels from 0 to 6 and I think they behave more clearly compared to the old outputs. |
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.
Thanks, @wandadars, I think the logging output at higher levels is quite a bit more readable now. I did have a couple thoughts about adjusting what gets added at each level, in contrast to the current levels.
One change here is that the information about what components are hitting their bounds is pushed to a higher loglevel -- from 5 for steady and 6 for transient solves to 6 and 7. While I don't think this is inherently a problem, it means that you aren't getting the ones for the transient solves until you are also generating a debug_sim1d.yaml
file, which I don't think we want. I would suggest either moving these outputs back to their current level, or bumping the log levels for the output file up one more level.
Also, it seems like you have to go to fairly high log levels (4+) to get any information about the Newton iterations, and that comes after the (very verbose) outputs of the system state after each steady state solve that comes with log level 3. I think the current approach where the abbreviated info on Newton iterations comes in at loglevel 2 makes more sense, at the same time that we're providing an abbreviated summary of each timestep. We could also consider deferring the system state printout to a higher debug level.
Thank you for the review comments @speth . One thing that was in my mind while adjusting these print statements was the question: "What is the guiding principle of the log levels here?" One thing that I tried to do was if a function gets a log level of 1, then it can be succinct in its summary because the statements don't have to worry about lower-level function calls injecting their own print statements into the flow of things. If the log level is higher than that, then the outputs at that level need to be expressed in such a way that they can still be understood even with a bunch of extra statements in-between. I adjusted where the loglevel was decremented ( I had it essentially decrementing during each nested call down into a function from the highest solve() level before). We have the steady newton summary and the timestep at logleve 2 now. I moved the full state output to one level higher. |
Yes, I understood that to be one of your main goals with the modifications to the logging, and I think what you have here is an improvement in that regard, by making some of the individual log records more self-contained and by the adjustments to the formatting of the "tabular" output. |
afd0086
to
d28d70d
Compare
…dn't already exist
… constants into MultiNewton class
d28d70d
to
b9fad36
Compare
b9fad36
to
216ffd8
Compare
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.
Thanks, @wandadars, this looks good to me.
@speth I have that script that parses and plots the output from the debug_sim1d.yaml file. Is that something worth including somewhere in Cantera? It's probably more of a tool for developers versus users. |
Do you have a link to the script? I think you posted some version of it in one of these threads, but I can't find it now. I think it would be worth preserving in some form. Perhaps on a page within the "Develop" section of the docs (https://cantera.org/dev/develop/index.html) along with some other suggestions on how to dig into unexpected issues with using the 1D code. |
For reference (when I get around to adding a page on the script), this is the script.
|
Current work for a potential improvement for the damped newton step function as well as an improvement in the debugging output that the 1D solver generates when loglevel is set to 8. For the debugging outputs, a more comprehensive history of the solution steps that result in a failure is kept in the debugging file, which can then be examined to find where a solution started to diverge
Checklist
scons build
&scons test
) and unit tests address code coverage