CL Solver Improvements, take 2 #278
Conversation
Codecov Report
@@ Coverage Diff @@
## main #278 +/- ##
==========================================
+ Coverage 40.86% 42.51% +1.64%
==========================================
Files 13 13
Lines 3901 3952 +51
==========================================
+ Hits 1594 1680 +86
+ Misses 2307 2272 -35
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I just updated the PR description to reflect the current changes that I want to merge. |
adflow/pyADflow.py
Outdated
| resultsDict : dictionary | ||
| Dictionary that contains various results from the cl solve. The | ||
| dictionary contains the following data: | ||
| >>> { |
There was a problem hiding this comment.
This doesn't render correctly. I think using bullets instead of >>> should work
There was a problem hiding this comment.
How do I use bullets?
There was a problem hiding this comment.
Something like:
* converged : bool
Flag ...
* iterations : int
…2 target. addressed PR comments
|
Thanks for the review, @sseraj. After our discussion with you and @lamkina the other day, I decided to remove as many of the ADflow options we were overwriting in the CL solver. This just results in some confusing behavior I think. Right now, I am only changing the relative L2 target, because that does need to be modified between each iteration. Other than this, if anybody wants a different ADflow option, they will need to make that change before going into the CL solver. I doubt many people are using the solver right now. Also, I think removing these options and defaults will cause less headache down the line when we want to change some defaults etc. |
|
|
||
| # put back the modified options | ||
| for option, value in modifiedOptions.items(): | ||
| self.setOption(option, value) |
There was a problem hiding this comment.
I agree that removing the options as arguments is cleaner. This part of finalizeSolver can also be removed, right?
There was a problem hiding this comment.
Right now, I am only modifying the relative L2 tolerance. However, keeping that finalize call there might be good because if we want to modify any other option down the line, we can easily track it the same way. The previous implementation had individual resets for each modified option. I say we keep it but I am also fine with changing it to an relL2 specific reset.
adflow/pyADflow.py
Outdated
| resultsDict : dictionary | ||
| Dictionary that contains various results from the cl solve. The | ||
| dictionary contains the following data: | ||
| >>> { |
There was a problem hiding this comment.
Something like:
* converged : bool
Flag ...
* iterations : int
Purpose
This PR completely changes the CL solver in ADflow. The new features are documented as docstrings in the new method, so I won't bother duplicating the explanations here. There are some tests that test the new capability.
Expected time until merged
2-4 weeks
Type of change
Testing
Checklist
flake8andblackto make sure the Python code adheres to PEP-8 and is consistently formattedfprettifyor C/C++ code withclang-formatas applicable