Generalized preconditioning and surface chemistry jacobian #1411
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1411 +/- ##
==========================================
+ Coverage 69.90% 69.96% +0.05%
==========================================
Files 377 377
Lines 57349 57533 +184
Branches 19201 19281 +80
==========================================
+ Hits 40092 40253 +161
- Misses 14706 14727 +21
- Partials 2551 2553 +2
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@speth while I still need to add more tests for code coverage. I believe the code is in a state that is ready for review when you have sometime. |
There was a problem hiding this comment.
Hi @anthony-walker ... great to see analytical derivatives extended to InterfaceKinetics! I left some comments on the Kinetics portion (which I know well due to my past work for the equivalent GasKinetics derivatives and the refactoring I did for the reaction rate evaluators). I won't comment on the reactor portion, which I'm less familiar with.
My main point in this review is that what you implemented here is exact for some simple surface reaction rate types, but derivatives may give unexpected results for many other rate expressions (for preconditioning this may be acceptable, but for other applications this would be misleading). I think it is necessary to clearly mark what is implemented, provide for some switches that enable/disable approximations, and raise warnings or errors as appropriate. @decaluwe may have some input on what to expect in terms of deviations from gas kinetics.
Finally, it would be good to see the correctness of derivatives checked for those cases where the analytical derivatives are known. For GasKinetics, I built an extensive test suite to make sure that results are correct (and to debug along the way); you can likely recycle some of these tests as appropriate.
There was a problem hiding this comment.
I'm glad to see the preconditioned solver being extended to surface chemistry.
I think there are some substantial changes in here that need to be explained in a bit more detail. Do you have a document with the mathematics behind the changes to the implementation that you can share? The translation of the formulas in your Symposium paper to the existing implementation was non-trivial, and it's not immediately obvious to me what's changing here and whether it's equivalent or not.
One specific change that I think is going to introduce more confusion than not is the use of "ddC" to mean derivatives with respect to each species concentration, rather than the total concentration (molar density) that it means right now.
anthony-walker
force-pushed
the
sdev
branch
8 times, most recently
from
73c85a5 to
33500f6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
anthony-walker
requested review from
speth and
ischoegl
and removed request for
speth and
ischoegl
|
@speth Thank you for the quick turn around on review again. I have made all of the requested updates. The two of most importance I believe were |
There was a problem hiding this comment.
Thanks for the updates, @anthony-walker. Seems like this is just about finished. I pushed an update to your branch with a couple of small modifications. Please pull that branch before addressing the few remaining minor issues below.
|
@speth Done! |
like IdealGasConstPressureMoleReactor
`_ddN` was used for concentration based derivatives now. Missing derivatives for surface chemistry were added which required some refactoring of mole reactor interfaces and updates to those derivatives. Some documentation was added and some of the requested PR changes were handled.
MoleReactors have varying orders of magnitude in comparsion to mass fraction based Reactors. Tolerances that are too tight based on the state vector cause test failures. This commit inherits the test and modifies tolerances appropriately.
This commit fixes derivatives for IdealGasConstPressureMoleReactor and updates IdealGasMoleReactor accordingly.
The corrections cause the matrices to become much more dense which greatly dimishes improvements due to sparsity. It also adds routines to scale the derivatives by appropriate terms based on the phase.
This commit updates the way the Jacobian is translated to the reactor jacobian and fixes the bug that causes difference by a factor of area and volume. A test is added to compare two reactors with various bulk phases and different orders of the phases.
Updating documentation, notation, renaming, etc. It also fixes some bugs that were founded. It finally adds a couple of methods to specify if a surface has certain reaction types
The indexing has been fixed and works with multiple surfaces correctly avoiding non-interacting phases. Tests were added to compare simple surface jacobians, gas phase jacobians, and single reactions. Removed surface contribution to energy equation terms as these contributions are not currently included in the system of equations and caus spurious entries.
`test_net_rop_ddCi` was updated to use and account for reaction orders appropriately. Many updates to docstrings and comments. Typo fixes. usesCoverageDependence was also implemented in a more generic manner so it can be extended to other reaction types.
There was a problem hiding this comment.
Thanks, @anthony-walker! I think this is good to go. I pushed a couple of minor formatting tweaks. We can merge this once the CI finishes.
|
@speth Great, glad to hear it! Thank you for the reviews and assistance through the process. |
Changes proposed in this pull request
Checklist
scons build&scons test) and unit tests address code coverage