Fix a few bugs and add tests: KineticsModel and uncertainty handling. #1705
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1705 +/- ##
=======================================
Coverage 32.61% 32.61%
=======================================
Files 87 87
Lines 26102 26102
Branches 6869 6869
=======================================
Hits 8514 8514
+ Misses 16629 16618 -11
- Partials 959 970 +11
Continue to review full report at Codecov.
|
mliu49
force-pushed
the
catfix
branch
2 times, most recently
from
e37e29d
to
1cd2cd6
Compare
|
I went ahead and rebased this on top of master. I also incorporated futurization and RMG 3 changes into the relevant commits. I think this looks good, but perhaps we should have a second reviewer? |
|
Thanks for rebasing, Max. A quick glance looks good to me (I have no time for a thorough review). |
|
Thanks for the contribution, Dr. West. Thanks for the rebasing, Max. I went through the codes and they looked good to me. I ran the RMG-test and all tests related to this PR passed. I also implemented PEP-8 formatting to this PR to be consistent with other RMG code. |
|
Corrections for |
So far just StickingCoefficient and SurfaceArrhenius. Doesn't test everything. Doesn't pass.
I think this fixes #1703
Can we pickle and unpickle, reconstruct from __repr__, and does the isIdenticalTo method work? (The answer to the last two is currently No)
First, the Tmin and Tmax were being printed for Pmin and Pmax, and the Pmin was being printed as the comment. Second, the uncertainty was not being reported.
This method just checks that Tmin and Tmax are equal. But if they were equal, it used to return False. Now it returns False if one or other is None, or they're different. Note, however, that because the Quantity.equals method defaults to a 1% tolerance, that's 20K difference needed at 2000K.
|
@xiaoruiDong, could you take a final look and approve? I added another commit fixing an issue with |
Motivation or Problem
Issue #1703 describes a problem whereby
SurfaceArrheniusobjects can't be copied withdeepcopy(meaning they can't be loaded in a kinetics family, or pretty much used for anything).It was due to the new
uncertaintyattribute being left out of the__repr__or__reduce__methods.While fixing and debugging, and adding unit tests, I caught some other bugs.
The
KineticsModelhad some errors, also in the__repr__method, and the logic behind the isIdenticalTo() was broken and it'd return False when it should be True (which was causing the new unit tests I wrote for the surface kinetics module to fail).Description of Changes
Added some unit tests that initially failed, and then fixed the bugs that they demonstrated.
Testing
It was test-driven development. A test I wrote to isolate one bug in fact led to finding two more.
Tested locally. Running Travis now. Test coverage around these modules is still pretty low, and even when lines of code are executed, the logic is not always being tested that the lines of code do the right thing. But improving that situation should wait until after Py3!