-
Notifications
You must be signed in to change notification settings - Fork 17
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
Fix/arrhenius kc #136
Fix/arrhenius kc #136
Conversation
By the way, if ever the kinetics rate tests can be a little bit more standardized and upgraded, please shout it there, I don't know why I didn't do it correctly in the first place, so if I can have them proper in the second place, that'd be great. |
c3059ed
to
959a533
Compare
merge 0.3.0-release into master, rebased this PR. |
can we have a merge of 0.3.0 into master and then this branch on master (after a rebase, I like rebases when they're easy)? I'll need it for PR #142. |
Done. |
Now all the tests are standardized, testing both KineticsConditions and StateType for the temperature. All the vectorizations are also tested.
Agreed. That's my MO on this - rebase is preferred if not terribly difficult. If it's really bad, fall back to merge to limit possibility of negatively rewriting history. |
959a533
to
484731e
Compare
rebased and happy on my computer |
I'm good with this, but I don't see the "slight bug fix", I only see the added overloads. What was the bug fix? |
That's the bug fix, thing is, when using a Edit: For the new added sensitivity stuff that is. |
Thanks for that remark, I'm missing the most important part of this bugfix: the Correcting this right away. |
Replaced by #182. |
This is both a slight bug fix and a cleaning/bettering of the tests.
For some reason that eludes me, I haven't made the overload with the
KineticsConditions
for all the rate constants' models computation.So this PR corrects this, and update the tests because it upset me that they were not already all standardized.