-
Notifications
You must be signed in to change notification settings - Fork 23
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
Adding Stoichiometric Coefficients #514
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #514 +/- ##
=======================================
Coverage 52.05% 52.05%
=======================================
Files 17 17
Lines 2096 2096
Branches 166 166
=======================================
Hits 1091 1091
Misses 933 933
Partials 72 72 ☔ View full report in Codecov by Sentry. |
I haven't tested myself but it looks okay to me. A suggestion for a test. Using
The results of the test should be identical to the results of |
I added a test of the stoichiometry here, but it is failing. To make the test, I copied the spec_model_1 directory and manually edited the mechanism file along with Looking at the files from the test, there only seems to be some small differences in the I don't think it's unreasonable to accept small differences between using the split mechanism and the stoichiometric coefficient mechanism since the problem being passed to the solver is slightly different, but I wanted to check that it would be OK to adjust this test to use the output from my stoichiometric model rather than comparing to the exact output from spec_model_1? Note that I have also made changes to the .kpp -> .fac conversion ( c34cdaa ) to allow my kpp test to complete as my filepath contains points ('.'), so was causing errors in trying to create the new kpp file. |
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 am satisfied that the differences between spec_model_1
and spec_model_stoich
are negligible. This can be merged, thank you!
I have looked into adding stoichiometric coefficients following discussions in #513. It turns out there was already a coefficient assigned in
src/inputFunctions.f90
for each species in each reaction, it was just being set to 1 every time. This coefficient of 1 was then already included in calculations in theresid
subroutine insrc/solverFunctions.f90
. This made it fairly easy to modify the code to have variable coefficients!I have made the following adjustments:
build/mech_converter.py
. If no coefficient is given then assign a coefficient of 1.mechanism.reac
andmechanism.prod
.src/inputFunctions.f90
instead of assigning a value of 1 for all species.I've run some models to test this and it seems to be behaving well. I've compared against the previous version with species duplicated and the results are identical. I've also tested some edge-cases like setting the coefficient to 0 and non-integer reactant coefficients, which also work as intended.
Let me know if there's any more changes I need to make, otherwise this resolves #513.