Non-cat changes to master from cat branch, 2 #1542
Conversation
rwest
force-pushed
the
catemp2
branch
3 times, most recently
from
fac8d84
to
7bead49
Compare
Codecov Report
@@ Coverage Diff @@
## master #1542 +/- ##
==========================================
- Coverage 41.99% 41.86% -0.14%
==========================================
Files 165 165
Lines 27896 28010 +114
Branches 5680 5714 +34
==========================================
+ Hits 11715 11726 +11
- Misses 15395 15496 +101
- Partials 786 788 +2
Continue to review full report at Codecov.
|
rwest
force-pushed
the
catemp2
branch
3 times, most recently
from
602b7b9
to
610f693
Compare
| @@ -785,7 +785,7 @@ cdef class ReactionSystem(DASx): | |||
|
|
|||
|
|
|||
| snapshot = [self.t, self.V] | |||
| snapshot.extend(y_coreSpecies / numpy.sum(y_coreSpecies)) | |||
| snapshot.extend(y_coreSpecies) | |||
There was a problem hiding this comment.
I agree analyzing moles work better for the general case and it's not difficult to convert them into whatever you need later during analysis. This won't have an impact on sensitivity analysis, since the sensitivity coefficients are converted from molar to concentration during simulation. It also doesn't look like it will impact flux diagram generation since it just pulls the concentrations off the object each time step rather than reading an output file.
rmgpy/solver/simple.pyx
Outdated
| @@ -154,6 +154,7 @@ cdef class SimpleReactor(ReactionSystem): | |||
| initialMoleFractions[speciesDict[label]] = moleFrac | |||
| self.initialMoleFractions = initialMoleFractions | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
It looks like this change was already made on master?
There was a problem hiding this comment.
Oops. If there had been no change the rebase would have dropped the commit. Will fix.
|
|
||
| #count up number of bonds | ||
| single = 0; rDouble = 0; oDouble = 0; sDouble = 0; triple = 0; benzene = 0 | ||
| single = 0; rDouble = 0; oDouble = 0; sDouble = 0; triple = 0; quadruple = 0; benzene = 0 |
There was a problem hiding this comment.
Since benzene doesn't have quadruple bonds, this commit message is a bit confusing.
rmgpy/molecule/molecule.py
Outdated
| @@ -1581,7 +1589,7 @@ def find_H_bonds(self): | |||
| atm_cov = atm_covs[0] | |||
| if (atm_cov.isOxygen() or atm_cov.isNitrogen()): #this H can be H-bonded | |||
| for k,atm2 in enumerate(ONatoms): | |||
| if all([q.order != 0 for q in atm2.bonds.values()]): #atm2 not already H bonded | |||
| if all([q.order != 0.1 for q in atm2.bonds.values()]): #atm2 not already H bonded | |||
There was a problem hiding this comment.
Should this be numpy.isclose(0.1,q.order) as well?
rmgpy/molecule/adjlist.py
Outdated
| @@ -89,7 +90,9 @@ def check_partial_charge(atom): | |||
|
|
|||
| theoretical = valence - order - atom.radicalElectrons - 2*atom.lonePairs | |||
|
|
|||
| if atom.charge != theoretical: | |||
| if np.isclose(-0.1, theoretical): | |||
There was a problem hiding this comment.
This will break if the molecule has more than one H bond? I don't expect having more than 2-
H bonds on a realistic molecule. Perhaps we should just check that abs(theoretical) > 0.301 before raising the error.
There was a problem hiding this comment.
Wouldn't it require an atom to have more than one H bond to break this?
But anyway, it's easy to do your suggestion.
Unsure if this has consequences on flux diagrams or sensitivity analyses, etc. but I think it's a helpful start towards making more sense of simulation outputs, especially for two-phase heterogeneous systems.
A combination of commits with these messages: replaced part of the adjlist in the test that I accidentally deleted added in a quadruple bond to orders Added in quadruple bond type in converter added in more instances of quadruple bonds Adding Cq to R and R!H atom types added in todos to add unit tests for quadruple bonded elements
It's unfortunate how fragile this code is, using integer indexing. Had to change getFeatures to include Benzene in its corrcect spot,
A combination of several commits, with these messages: changed the remove H bonds to remove anything close to 0.1 Hydrogen-bonds now have order 0.1 This way of doing things is a bit fragile, and not very satisfying. But I think this fixes at least a couple of places where this change needs to be made. Detect 0.1 bond orders (for H bonds) more robustly. Now passes last unit test. (It was something like 0.1000000000000149)
Reaction recipes can make Van der Waals bonds (with order 0) Remove 'vdW' from bond types accepted (should be 0). This is a hold-over from when bond types were strings not floating point numbers.
Might not make a difference if you could see the logging.info call two lines earlier, but when running unit tests (databaseTest) that is not necessarily the case.
|
I believe I addressed all @mjohnson541's comments, and the tests seem to be passing (I think the coverage check doesn't detect cythonized stuff properly?). |
Motivation or Problem
This is a follow-on from #1538.
This is the second of two pull requests that don't actually introduce surface sites or heterogeneous reactions, but contains a series of commits that prepare the groundwork for merging RMG-Cat (heterogeneous catalysis) features into RMG.
Description of Changes
Testing
The last change (tracking moles not mole fractions) may mess up some regression tests, depending how they're configured, and should come with warnings (that the meaning of RMG's output has changed), but I think it's an improvement. I am open to discussion though, if people feel it should be an configurable option or something. Mole amounts make so much more sense when you have surfaces etc. And if you want to get a mole fraction, it's trivial to go in that direction.
Reviewer tips.
Now that #1538 has been merged, and all the unit tests and regression tests seem to be passing, I guess it's ready for review.