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

BugFix: Empty list as val in RMGObject.as_dict() and .make_object() #1540

Merged
merged 2 commits into from
Jan 27, 2019

Conversation

alongd
Copy link
Member

@alongd alongd commented Jan 23, 2019

Motivation or Problem

When RMGObject encounters an empty list, it cannot index it (e.g., call val[0]). This occures when running atomic species where the frequencies are just an empty list [].

Description of Changes

We shouldn't bother dumping and parsing empty lists, they will get their default values when loading the object.

Testing

Files for testing (for carbon atom) are attached
C_arkane_input.txt
input.txt
sp.txt

Reviewer Tips

After successfully generating a YAML file for C, try feeding that YAML file back to Arkane to see that it reads it correctly.

@alongd alongd added the Arkane label Jan 23, 2019
@alongd alongd self-assigned this Jan 23, 2019
@alongd alongd requested a review from mliu49 January 23, 2019 14:17
@codecov
Copy link

codecov bot commented Jan 23, 2019

Codecov Report

Merging #1540 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1540   +/-   ##
=======================================
  Coverage   42.02%   42.02%           
=======================================
  Files         165      165           
  Lines       27867    27867           
  Branches     5680     5680           
=======================================
  Hits        11711    11711           
  Misses      15371    15371           
  Partials      785      785

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 3a7b901...0ea86db. Read the comment docs.

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.

Looks good!

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.

Actually, could you add a simple unit test?

@mliu49
Copy link
Contributor

mliu49 commented Jan 24, 2019

I added a new test module for RMG object. One of the tests is actually failing, which I'm glad was caught. If the value is a numpy array, then directly checking the truth value is ambiguous and raises an exception.

@alongd
Copy link
Member Author

alongd commented Jan 25, 2019

Great catch! Thanks for adding this test

@mliu49
Copy link
Contributor

mliu49 commented Jan 25, 2019

The fix looks good! You can go ahead and squash.

alongd and others added 2 commits January 25, 2019 11:18
@alongd
Copy link
Member Author

alongd commented Jan 25, 2019

done!

@mliu49 mliu49 merged commit ef169bb into master Jan 27, 2019
@mliu49 mliu49 deleted the rmgobject branch January 27, 2019 04:32
@mliu49 mliu49 mentioned this pull request May 15, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants