Skip to content

Fix: numeric energy input for ARKANE#1811

Closed
jeehyunatrmg wants to merge 1 commit into
masterfrom
jeehyun_110519
Closed

Fix: numeric energy input for ARKANE#1811
jeehyunatrmg wants to merge 1 commit into
masterfrom
jeehyun_110519

Conversation

@jeehyunatrmg

Copy link
Copy Markdown

Motivation or Problem

I wanted to manually put energy in Arkane input file since Arkane doesn't contain model chemistry that I used. However, Arkane assumed float type as 'path' and giving out the error.

Description of Changes

added lines that can recognize the float type before it assumes as the path

Testing

it worked in my laptop..?

Reviewer Tips

Actually, Jerry did this fix...

Comment thread arkane/statmech.py
if not os.path.isfile(energy.path):
if not hasattr(energy, 'path'):
pass
elif not os.path.isfile(energy.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.

I would suggest putting the check in the same statement: if hasattr(energy, 'path') and not os.path.isfile(energy.path)

Also, I'm not sure if Alon has opinions on use of hasattr vs checking isinstance(energy, Log).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think isinstance(energy, Log) is cleaner, and it should definitely be in the same statement.
@jeehyunatrmg, we can do it together tomorrow if you'd like, or simply go-ahead and make these fixes.

@goldmanm

goldmanm commented Nov 6, 2019

Copy link
Copy Markdown
Contributor

I also worked on a similar issue, which is in the PR Arkane bugfixes #1810 . You can see the change at this commit:
cba4f3f

@codecov

codecov Bot commented Nov 6, 2019

Copy link
Copy Markdown

Codecov Report

Merging #1811 into master will decrease coverage by 10.15%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1811       +/-   ##
===========================================
- Coverage   42.74%   32.58%   -10.16%     
===========================================
  Files          81       98       +17     
  Lines       21078    27665     +6587     
  Branches     5491     7254     +1763     
===========================================
+ Hits         9010     9016        +6     
- Misses      11067    17640     +6573     
- Partials     1001     1009        +8
Impacted Files Coverage Δ
rmgpy/data/statmech.py 42.2% <0%> (ø) ⬆️
rmgpy/rmg/pdep.py 12.21% <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/statmech/ndTorsions.py 59.78% <0%> (ø) ⬆️
.../test_data/testing_database/forbiddenStructures.py 100% <0%> (ø)
rmgpy/molecule/molecule.py 0% <0%> (ø)
rmgpy/species.py 0% <0%> (ø)
... and 14 more

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 6b2555a...2f50771. Read the comment docs.

@alongd

alongd commented Nov 6, 2019

Copy link
Copy Markdown
Member

We can either use this PR or @goldmanm's commit. This PR could also be a good practice. Either way we should also add a test.

@amarkpayne

Copy link
Copy Markdown
Member

What is the current status of this PR? Are we using the change here or are we using the commit from @goldmanm ?

@goldmanm

Copy link
Copy Markdown
Contributor

9509839 fixed this issue.

@goldmanm goldmanm closed this Nov 18, 2019
@jeehyunatrmg
jeehyunatrmg deleted the jeehyun_110519 branch November 18, 2019 21:06
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.

5 participants