search

mutationpp / Mutationpp

The MUlticomponent Thermodynamic And Transport library for IONized gases in C++
GNU Lesser General Public License v3.0
108 stars 58 forks source link

Issue 74: Bugs/segfault in jacobianRho() #96

Closed jbscoggi closed 5 years ago

jbscoggi commented 5 years ago

This PR fixes #74. Namely, it solves 3 issues found in the Kinetics::jacobianRho() function:

  1. Fixes the cause of a segfault when the number of reactions is less than the number species,
  2. Removes electrons from consideration as a third-body for third-body reactions, and
  3. Removes redundant Jacobian contribution from third-body species in reactions that are not explicitly third-body reactions (e.x.: N2 + N = N + N + N).

In addition to fixing these bugs, the test_reaction_mechanism.cpp file is updated to ensure the Jacobian is tested as well against a hard-coded mechanism implementation.

Note that this is a draft for now because the proposed fixes are slow. A few optimizations will be put in place before allowing this to be merged.

jbscoggi commented 5 years ago

After the last commit, the code has been cleaned up and some redundant code removed, for a small improvement in performance (non exhaustive profile). I think this is a good point to review the code and merge as it successfully solves #74. We can have a dedicated issue for increasing performance later if we think it's necessary.

grgbellasvki commented 5 years ago

I think this addresses all of the issues (third-body and electrons) and the code seems clean. I will accept as soon as the tests pass. Next step: jacobians wrt Ts.

jbscoggi commented 5 years ago

OK cool, thanks. For the temperature Jacobian, can you add an issue so we don't forget?

grgbellasvki commented 5 years ago

I will, but before, I need to review what it is missing. For example, the derivative of kf with a single temperature is ok. Slight modifications are needed for the sqrt(TTv) case. I will post a picture including the formulas (not complicated, but a little lengthy).

jbscoggi commented 5 years ago

OK sounds good.

jbscoggi commented 5 years ago

I went ahead and merged because I wanted to check if it passed and saw it did. Not sure where the code coverage report went though...