Skip to content
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

Minor changes for ANK solver behavior #277

Merged
merged 124 commits into from
Mar 28, 2023
Merged

Conversation

anilyil
Copy link
Contributor

@anilyil anilyil commented Mar 27, 2023

Purpose

This PR contains the ANK-related changes from #246. The CL solver in that PR will need more work, but I want to clean up some of my work branches sooner than that.

The changes are:

  1. ANK CFL Reset: I added a python option "ANKCFLReset" that defaults to True. This was the original behavior of the solver, where at each subsequent ADflow call, the ANK CFL would be reset to "ANKCFL0" and then the moving lower bound was applied in the solver itself based on the L2 convergence. Now, the users can set "ANKCFLReset" to False, so that the last ANK CFL is preserved exactly as the solver goes into the next iteration. This improves performance near the final stages of optimizations, or with CL-solvers where the problem does not change all that much. However, restarting the new case at high CFL numbers can make the solver unstable, so I am keeping the original behavior as default. The ANK CFL for each case is saved in the aeroProblem's ADflow data.

  2. ANK Linear Solver Buffer: This was a hard-coded value in fortran, which is now exposed to Python. This determines how much we over-solve the linear system when we are near the L2 convergence target. Not really relevant for most people, only really comes in handy when you are trying to do performance tests and you would prefer the linear solver to quit as soon as you think you are near the L2 target. This is a linear approximation, so achieving the L2 target is not guaranteed. That's why we multiply the L2 target by the linear solver buffer "ANKLinearSolveBuffer", which defaults to 1e-2. Increasing this value to something like 0.5 would be a bit more accurate at the risk of running a few extra iterations due to the linear approximation not being an exact calculation of the nonlinear residual.

  3. Added a new PC update tolerance: The "ANKPCUpdateTolAfterCutoff" option can be used to control the PC update based on L2 convergence after the "ANKPCUpdateCutoff" is reached. Again, mostly a performance related option that most people don't need to worry about.

  4. Added method to get number of cells: The getNCells method in pyADflow returns the total number of cells in the grid as computed in the fortran layer.

Expected time until merged

Would be great if this is done within a week.

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Checklist

  • I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

anilyil and others added 30 commits January 30, 2022 20:07
…rgence. also fixed the mgpc setup for ank turb ksp
@anilyil anilyil requested a review from a team as a code owner March 27, 2023 06:17
@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #277 (e9262f3) into main (9874e80) will increase coverage by 0.10%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #277      +/-   ##
==========================================
+ Coverage   40.77%   40.87%   +0.10%     
==========================================
  Files          13       13              
  Lines        3882     3892      +10     
==========================================
+ Hits         1583     1591       +8     
- Misses       2299     2301       +2     
Impacted Files Coverage Δ
adflow/pyADflow.py 66.68% <80.00%> (+0.05%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Just some minor comments

adflow/pyADflow.py Outdated Show resolved Hide resolved
doc/options.yaml Show resolved Hide resolved
@anilyil anilyil requested a review from sseraj March 28, 2023 06:34
sseraj
sseraj previously approved these changes Mar 28, 2023
Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

Cool, just a couple of clarifications. I use coupled ANK so I guess I will need to change the default PC update tolerance option after this?

adflow/pyADflow.py Show resolved Hide resolved
This option only really makes a difference very close to the final L2 convergence target.
For example, setting a value of 0.01 will force the linear solver to converge the linear system more tightly, in the cases where the absolute linear convergence is achieved before the relative target.
Reducing the value here to 1e-4 or 1e-6 may result in "extra" nonlinear convergence in the final iteration, at the cost of more linear solver effort.
Increasing the value here to 1e-1 will reduce this buffer and the solver is more likely to end closer to the final L2 convergence target, but a too large of a value might cause issues with the final iterations where several smaller additional Newton steps are required.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth mentioning the "performance improvement" motivation here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don't want to mention it here. People should not use this unless they know what they are doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow up comment: The reason I dont want to mention is that setting this variable too high has the potential of tripping the linear solver near the end so you never reach the L2 convergence. If the user does not understand what this option does exactly (which is fairly nuanced), then its likely they wont be able to fix this issue if it arises.

Effectively, this is only really useful for solver performance tests for papers etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

that makes sense, I was not thinking to encourage users to play with this, but rather explain this. Maybe a heads up explicitly saying "People should not use this unless they know what they are doing" could be useful? No strong opinions on my hand here, your call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok added a warning

doc/options.yaml Show resolved Hide resolved
@anilyil
Copy link
Contributor Author

anilyil commented Mar 28, 2023

"Cool, just a couple of clarifications. I use coupled ANK so I guess I will need to change the default PC update tolerance option after this?"

This stuff was added relatively recently in #231, so I doubt this was affecting you. If you want you can change this option, but only do it if you understand what is going on and if your case is doing too many PC updates. This level of performance optimization is usually irrelevant for academic work, so I would not bother most likely. Getting a more robust solver is always better, so I recommend not changing the default.

@marcomangano
Copy link
Contributor

This level of performance optimization is usually irrelevant for academic work, so I would not bother most likely. Getting a more robust solver is always better, so I recommend not changing the default.

Perfect, thanks!

@anilyil anilyil requested a review from sseraj March 28, 2023 18:28
@sseraj sseraj merged commit 5218194 into mdolab:main Mar 28, 2023
@anilyil anilyil mentioned this pull request Mar 29, 2023
13 tasks
@anilyil anilyil deleted the ank_cfl_reset branch March 29, 2023 08:12
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.

3 participants