-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Small fixes and improvements / sparse coefficient matrices #1088
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1088 +/- ##
==========================================
+ Coverage 73.46% 73.49% +0.03%
==========================================
Files 364 364
Lines 47667 47809 +142
==========================================
+ Hits 35018 35138 +120
- Misses 12649 12671 +22
Continue to review full report at Codecov.
|
2c0cc44
to
1b11a99
Compare
'scons help' reports setting values in terms of 'yes' and 'no' rather than boolean flags that are otherwise used internally. This commit ensures that documentation for the newly added flag 'legacy-rate-constants' is consistent with this convention; accordingly, the setting is either enabled or disabled.
Compilation with GNU g++ currently raises the following warning. warning: 'nSolnValues' may be used uninitialized in this function [-Wmaybe-uninitialized] The warning is resolved by providing an (invalid) initial value; the value is overwritten by a subsequent (pre-existing) if-else tree.
Resolve GNU g++ warning: warning: unused variable 'p' [-Wunused-variable]
bd9dca4
to
04e6b1d
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.
I like the addition of the option to use sparse matrices for the stoichiometric coefficients, but I think there are a few tweaks needed to get it to make full use of the sparsity.
The one change here that I don't really understand is to the data structure used to initialize Plog
objects.
@speth ... thank you for the comments. I'll respond to some of your points right away, and should be able to get back to this before long. |
f6d7434
to
a969367
Compare
* Use consistent interface based on Array2D * Use column-major consistently in internal calculations * Deprecate Chebyshev::coeffs and Chebyshev::setup
Several internal methods defined for C1, C2, C3, and C_AnyN are unused; as there are no simple accessor methods defined in StoichManagerN, deprecation appears not to be required.
The 'Kinetics::finalizeSetup' method executes code after all reactions have been added. The new flag 'finalize' is added to 'Kinetics::addReaction' which determines whether 'finalizeSetup' is executed. While the default is 'true', imports within 'KineticsManager' use 'addReaction' with the flag set to 'false'.
as Kinetics::m_irrevProductManager is only used in conjunction with Kinetics::m_revProductManager, a new Kinetics::m_productManager provides for simpler code.
Deprecate Tmin/Tmax, Pmin/Pmax as well as constructors to use std::pair<double,double> input/output
Deprecate Kinetics.*_stoich_coeffs() method in favor of property: Behavior will change after Cantera 2.6; for new behavior, new properties Kinetics.*_stoich_coefficients are implemented
This commit unifies parameter handling of Plog rate setters and getters to use std::multimap<...>.
a969367
to
79e62d8
Compare
@speth ... thanks again for the feedback (and I'm happy you liked the sparse coefficient interface, despite the naming squabbles). I believe I took care of everything you mentioned thus far. The interface to One new significant change is that I streamlined
with corresponding changes in the C++ underpinnings (setters/getters/constructors). Edit: Writing this, I realized that
Finally, the transitional names just end with
|
🎉 ... finally found something that will help exposing sparse matrices without copying ... it does look like scipy.sparse.csc_matrix can be initialized from the data structure used by compressed sparse matrices in
|
Switching to CSC (from COO) format in
These tests show a 20% speed improvement with CSC (not a major saving); CSC is, however, better suited for matrix-vector products, so the switch makes sense overall. |
This change leverages the default internal storage format in Eigen (CSC)
41338c1
to
5394540
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 for the updates, @ischoegl. I'm glad to see the reasonably efficient interface for exposing Eigen's sparse matrices to Python. I think that bodes well for some of the other things that I expect are on the horizon like Jacobians.
The change from Plog::setup
to Plog::setRates
makes sense, although I think the new function name implies that you could call it again to replace the existing rates and the current implementation doesn't support that as it leaves the existing contents of rates_
and pressures_
in tact.
I don't really understand the motivation for packing Chebyshev rate Tmin/Tmax and Pmin/Pmax into std::pair
. All the extra use of std::make_pair
and foo.first
/foo.second
that's now required doesn't really seem like an improvement over the old interface. In Python, on the other hand, implicit tuple packing/unpacking is clean and idiomatic, so I think that's a reasonable API change.
7d37cc1
to
85f019d
Compare
85f019d
to
202c2bf
Compare
@speth ... thank you for your comments!
Good catch! Some of the renaming here pre-empts #1051 (which is currently on hold), so it will be possible to change that eventually even in memory.
I partially reverted those changes to C++. I guess I wanted to make things as similar as possible for YAML / Python and C++, but for the latter I agree that the resulting API change wasn't great. The updated Python API remains of course. Regarding |
@speth ... after settling on |
Update nomenclature for previously introduced finalizeSetup method; new names are Kinetics::resizeReactions and StoichManagerN::resizeCoeffs
976527e
to
c4e4a7b
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, @ischoegl! This looks good to me.
@speth ... thanks again for the review. Now that this is merged, I will post a new PR wrapping up None of my other pending PR's are close to being merged, as most need to be rebased at this point. |
Changes proposed in this pull request
This PR contains several minor updates that fix small issues accumulated over several PR's (or address inconsistencies that were left over from the 2.5 code base).
Plog
interface consistent: existing code mixedstd::vector<std::pair>
andstd::multimap
(deprecatestd::multimap
versions)Chebyshev
interface consistent: existing code mixedArray2D
andvector_fp
(deprecatevector_fp
versions)Chebyshev
routines to consistently use column major arrayslegacy_rate_constants
NotImplementedError
messages (allows for more granular user feedback)eigen_dense.h
toeigen_defs.h
(prepares for use of sparse matrices)StoichManagerN
) viaEigen::SparseMatrix
scipy.sparse
Most of these changes are straight-forward; some are meant to preempt questions that may arise in the context of #1051 and #1081. Specifically, the sparse interface is a preamble to sparse Jacobian output for the Python API in #1081.
If applicable, provide an example illustrating new features this pull request is introducing
Almost all updates are internal. The main API change allows for sparse matrix output (implemented for stoichiometric coefficients):
Checklist
scons build
&scons test
) and unit tests address code coverageOther thoughts
I also considered using
Eigen::MatrixXd
for the externalChebyshev
interface (2-D matrix of coefficients), but ended up staying closer to the previous implementation.For simplicity, theEigen::SparseMatrix
interface to Python is implemented using a C preprocessor; the alternative would be to expose someEigen
classes in_cantera.pxd
.