A collection of Matt and David's commits from #2316 #2491
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2491 +/- ##
==========================================
+ Coverage 48.50% 48.55% +0.04%
==========================================
Files 110 110
Lines 30761 30812 +51
Branches 8039 8054 +15
==========================================
+ Hits 14920 14960 +40
- Misses 14320 14329 +9
- Partials 1521 1523 +2
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
The only test failures are expected failures of the regression tests because of the random run-to-run variations. |
rwest
force-pushed
the
matt
branch
2 times, most recently
from
6727b61
to
5883156
Compare
There was a problem hiding this comment.
@mjohnson541 isn't this "make it possible to explicitly forbid molecules and groups in the input file" feature already here thanks to #2185 ?
Besides adding a new, undocumented, name for it, not sure what this commit adds? Can you use the existing mechanism in your input files and we drop this commit?
I see you authored this commit a couple of weeks before PR #2185 was merged, so you're forgiven for not using it back then :)
… the input file" This reverts commit 92fad47. also "fig adjacencyListGroup in input" which was a fixup. The functionality has been available for 2 years since ReactionMechanismGenerator#2185 so adding a new syntax for doing it would be confusing and unnecessarry.
SurfaceChargeTransfer and ArrheniusChargeTransfer are not yet defined (on this branch). Revert this commin once they are.
For now it just tests Arrhenius types, but should eventually work on others.
maximumSurfaceSites and maximumSurfaceBondOrder
Doesn't make a real difference, just easier for a human to parse the code without making errors, when the "if" blocks are in the same order as the documentation and examples.
| count = 0 | ||
| for kinetics in kinetics_list: | ||
| count += 1 | ||
| logA += np.log10(kinetics.A.value_si) | ||
| n += kinetics.n.value_si | ||
| Ea += kinetics.Ea.value_si | ||
|
|
||
| logA /= count | ||
| n /= count | ||
| Ea /= count |
There was a problem hiding this comment.
We should just use Python's built in average, or numpy's. i.e. np.mean([i.n.value_si for i in kinetics_list]) (and include the log10 for geometric, or use a method that just does geometric mean).
There was a problem hiding this comment.
But we'd have to iterate through the list multiple times (once for each attribute) and create a bunch of lists. Is this a verbosity thing? an efficiency thing? a less-likely-to-make-mistakes thing?
I don't like playing code golf for the sake of it.
There was a problem hiding this comment.
But we'd have to iterate through the list multiple times (once for each attribute) and create a bunch of lists.
You can do it very efficiently in one line like this:
from types import SimpleNamespace
import numpy as np
# mimic the kinetics objects
kinetics_list = [SimpleNamespace(a=1, b=2, c=3) for _ in range(3)]
a, b, c = np.mean([[np.log10(i.a), i.b, i.c] for i in kinetics_list], axis=1)Is this a verbosity thing? an efficiency thing? a less-likely-to-make-mistakes thing? I don't like playing code golf for the sake of it.
This is an antipattern, which we benefit from removing for all of these reasons. Numpy will be faster, less code is easier to maintain and read, etc.
There was a problem hiding this comment.
A hole in one!
I quite like it. Although, having now taught programming a few times, I am learning to appreciate the benefits of code that a novice can understand at a glance. More participation, fewer mistakes, less time spent explaining or parsing, etc. etc.
Anyway, the main reason to not do it your way here and now is that we soon need to revert this commit
2180175
and the merge conflicts would be horrible to resolve.
We can revisit optimizing this code after that happens.
(Though I think this code is seldom called)
If you happen to pick the first of the list then the or or method was faster. But if you pick the last of the list (or not in the list) then the in tuple method is faster. Also changed some lists to sets for consistency, and because they're a touch faster. See pylint https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/consider-using-in.html (Change requested in code review) Also switched a format to an fstring.
Must be left over from the PEP8-ify project.
The only way r would be 'None' is if a symmetrical reaction (reactants == products) was found in a library, and hit this check in check_for_existing_reaction that's called by make_new_reaction. But that suggests some other problem. So I doubt this test is often needed. But Matt added it for something. I can't see any way it would be False, so now we check it's not None. (as requested in code review)
Unfortunately we need to revert the commit titled "Remove charge transfer types from average_kinetics (to be reverted)" at some point in the not so distant future, and doing this different averaging scheme now would mess that up.
|
Think I've addressed all your comments. I pushed back on one suggestion - though I like it - because it'll cause horrible merge conflicts soon because that method needs to be more complicated than it currently looks. But the rest I adopted. |
Motivation or Problem
Pull request #2316 which adds electrochemistry, adds lithium, adds solvation to surfaces, adds charge transfer reactions, is rather large and hard to review, and also has some necessarry but unrelated things, that can be helpful elsewhere.
Description of Changes
I've pulled out a bunch (but not all) of those things into this PR so they can be reviewed and merged, then the electrochem PR can be rebased onto it. I also reordered things so the fixups commits are mostly right after the things they're fixing.
One feature that was added was the ability to forbid species and functional groups in input files. As I was trying to add examples to the input files and documentation, I found it was already there, since this feature was added in #2185 - authored around the same time in October 2021. Rather than add a second syntax for the same thing, I reverted the commits on this branch.
Testing
I read the commits as I was cherry-picking, got the test suite to pass, and also added some more unit tests.