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

Several small Pdep bug fixes #1785

Merged
merged 3 commits into from
Oct 30, 2019
Merged

Several small Pdep bug fixes #1785

merged 3 commits into from
Oct 30, 2019

Conversation

alongd
Copy link
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

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
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.

@@ -297,9 +297,6 @@ def load_yaml(self, path, label=None, pdep=False):
self.imaginary_frequency = ScalarQuantity()
self.imaginary_frequency.make_object(data=freq_data, class_dict=class_dict)

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
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
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

@@ -104,7 +104,7 @@ cpdef generate_full_me_matrix(network, bint products=True):
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
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
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
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
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