Skip to content

Several small Pdep bug fixes#1785

Merged
alongd merged 3 commits intomasterfrom
pdep_bugs
Oct 30, 2019
Merged

Several small Pdep bug fixes#1785
alongd merged 3 commits intomasterfrom
pdep_bugs

Conversation

@alongd
Copy link
Copy Markdown
Member

@alongd alongd commented Oct 24, 2019

  • Corrected Network.eq_ratios to Network.eqRatios in cse
  • Fixed an incorrect value assignment line in ME
  • Removed an error raised in Arkane when loading YAML files generated without an energy transfer model when running a PDep job

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 24, 2019

Codecov Report

Merging #1785 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1785   +/-   ##
=======================================
  Coverage   32.61%   32.61%           
=======================================
  Files          87       87           
  Lines       26124    26124           
  Branches     6878     6878           
=======================================
  Hits         8521     8521           
+ Misses      16644    16633   -11     
- Partials      959      970   +11
Impacted Files Coverage Δ
rmgpy/pdep/network.py 12.45% <0%> (ø) ⬆️
rmgpy/data/statmech.py 42.2% <0%> (ø) ⬆️
rmgpy/rmg/pdep.py 12.21% <0%> (ø) ⬆️
rmgpy/reaction.py 0% <0%> (ø) ⬆️
rmgpy/data/kinetics/family.py 48.35% <0%> (ø) ⬆️
rmgpy/yml.py 15.71% <0%> (ø) ⬆️
rmgpy/data/kinetics/database.py 50.61% <0%> (ø) ⬆️
rmgpy/rmg/input.py 34% <0%> (ø) ⬆️
rmgpy/molecule/molecule.py 0% <0%> (ø) ⬆️
rmgpy/statmech/ndTorsions.py 59.78% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30553d7...c9c1d27. Read the comment docs.

@mliu49
Copy link
Copy Markdown
Contributor

mliu49 commented Oct 30, 2019

Perhaps we should change the eqRatios attribute to eq_ratios instead? I think it was missed during renaming because it's not in the Network.__init__ method.


if pdep and not self.is_ts and (self.transport_data is None or self.energy_transfer_model is None):
raise ValueError('Transport data and an energy transfer model must be given if pressure-dependent '
'calculations are requested. Check file {0}'.format(path))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this no longer necessary?

Copy link
Copy Markdown
Member Author

@alongd alongd Oct 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you take a look at the commit message? It explains it

if k_ij[i, j, n_grains - 1,0] > 0 or k_ij[j, i, n_grains - 1,0] > 0:
for r in range(n_grains):
for s in range(n_j):
u, v = indices[i, r, s], v = indices[j,r,s]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did this not break any tests??

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right?!? it's very concerning

even if running a pdep job
These attributes can be given in the Arkane species file, and this check crushes Arkane before loading additional species parameters. If these parameters are indeed missing, Arkane will crush anyway downstream.
Also fixes a bug where in cse.pyx we assign `eq_ratios = network.eq_ratios` (it is now correct)
@alongd
Copy link
Copy Markdown
Member Author

alongd commented Oct 30, 2019

I force pushed, reverting the original eq_ratios commit in cse, and modifying it in network instead, as you suggested.
also rebased

Copy link
Copy Markdown
Contributor

@mliu49 mliu49 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@alongd alongd merged commit 6c016f6 into master Oct 30, 2019
@alongd alongd deleted the pdep_bugs branch October 30, 2019 20:22
@mliu49 mliu49 mentioned this pull request Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants