Skip to content
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

Closed

Conversation

SylvainPlessis
Copy link
Contributor

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.

@SylvainPlessis
Copy link
Contributor Author

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.

@SylvainPlessis
Copy link
Contributor Author

merge 0.3.0-release into master, rebased this PR.

@SylvainPlessis
Copy link
Contributor Author

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.

@pbauman
Copy link
Member

pbauman commented May 1, 2015

an we have a merge of 0.3.0 into master

Done.

Now all the tests are standardized, testing both KineticsConditions and
StateType for the temperature. All the vectorizations are also tested.
@pbauman
Copy link
Member

pbauman commented May 1, 2015

(after a rebase, I like rebases when they're easy)?

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.

@SylvainPlessis
Copy link
Contributor Author

rebased and happy on my computer

@pbauman
Copy link
Member

pbauman commented May 2, 2015

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?

@SylvainPlessis
Copy link
Contributor Author

That's the bug fix, thing is, when using a KineticsConditions object for far up, if ever you end with one of those kinetics model, you get a compiling error.

Edit: For the new added sensitivity stuff that is.

@SylvainPlessis
Copy link
Contributor Author

Thanks for that remark, I'm missing the most important part of this bugfix: the KineticsType object still passed a const StateType & T to those not updated models.

Correcting this right away.

@pbauman
Copy link
Member

pbauman commented Feb 5, 2016

Replaced by #182.

@pbauman pbauman closed this Feb 5, 2016
pbauman added a commit that referenced this pull request Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants