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

Input Writer #188

Merged
merged 26 commits into from Mar 12, 2021
Merged

Input Writer #188

merged 26 commits into from Mar 12, 2021

Conversation

FarnazH
Copy link
Member

@FarnazH FarnazH commented Aug 20, 2020

This PR addresses issue #43.

  1. Thewrite_input is added to api module.
  2. The Gaussian input writer is added as an example and is tested with the default and user-defined input template.

evohringer
evohringer previously approved these changes Aug 20, 2020
Copy link
Contributor

@evohringer evohringer left a comment

Choose a reason for hiding this comment

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

It seems all clear to me. Great!

iodata/api.py Outdated Show resolved Hide resolved
iodata/inputs/gaussian.py Outdated Show resolved Hide resolved
iodata/inputs/gaussian.py Outdated Show resolved Hide resolved
iodata/inputs/gaussian.py Outdated Show resolved Hide resolved
Copy link
Member

@tovrstra tovrstra left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I just have a few minor comments...

iodata/api.py Outdated Show resolved Hide resolved
iodata/inputs/gaussian.py Show resolved Hide resolved
iodata/inputs/gaussian.py Outdated Show resolved Hide resolved
iodata/inputs/gaussian.py Outdated Show resolved Hide resolved


def write_input(f: TextIO, data: IOData, template: str = None):
"""Do not edit this docstring. It will be overwritten."""
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above regarding documentation decorator.

iodata/test/test_inputs.py Outdated Show resolved Hide resolved
iodata/test/test_inputs.py Outdated Show resolved Hide resolved
iodata/test/test_inputs.py Outdated Show resolved Hide resolved
iodata/inputs/orca.py Outdated Show resolved Hide resolved
Copy link
Contributor

@lmacaya lmacaya left a comment

Choose a reason for hiding this comment

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

Only minor comments, but I think the main functionality is complete. Work on the docstring is still pending, so I hope to collaborate on that this week.

fields["charge"] = int(data.charge) if data.charge is not None else 0

# convert run type to Gaussian keywords
run_types = {"energy": "sp", "freq": "freq", "opt": "opt"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Based in #202, we can update this dict to all the run_types supported in IOData.

fields["run_type"] = data.run_type if data.run_type is not None else 'energy'
# convert spin polarization to multiplicity
fields["spinmult"] = int(data.spinpol) + 1 if data.spinpol is not None else 1
fields["charge"] = int(data.charge) if data.nelec is not None else 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fields["charge"] = int(data.charge) if data.nelec is not None else 0
fields["charge"] = int(data.charge) if data.charge is not None else 0

fields["obasis_name"] = data.obasis_name if data.obasis_name is not None else 'STO-3G'

# convert run type to orca keywords
run_types = {"energy": "Energy", "freq": "Freq", "opt": "Opt"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as gaussian.py, we can update this dict.

Copy link
Member

@tovrstra tovrstra left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I've added a few small comments and I also have a few more general small requests:

  • Could you add the file header to iodata/inputs/__init__.py?
  • The initialization of the fields dictionary can be moved to a common.py module. This code can be reused by most formats.

(There are also some open comments from my previous review. If these are not clear, please let me know. I'll dig them up if needed.)

iodata/inputs/orca.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 2, 2020

Codecov Report

Merging #188 (0f628f8) into master (c8e8f10) will decrease coverage by 0.19%.
The diff coverage is 95.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
- Coverage   95.76%   95.56%   -0.20%     
==========================================
  Files          63       65       +2     
  Lines        6752     6772      +20     
  Branches      865      860       -5     
==========================================
+ Hits         6466     6472       +6     
- Misses        129      139      +10     
- Partials      157      161       +4     
Impacted Files Coverage Δ
iodata/api.py 86.11% <78.26%> (-4.09%) ⬇️
iodata/docstrings.py 96.66% <89.47%> (-3.34%) ⬇️
iodata/inputs/common.py 100.00% <100.00%> (ø)
iodata/inputs/gaussian.py 100.00% <100.00%> (ø)
iodata/inputs/orca.py 100.00% <100.00%> (ø)
iodata/test/test_inputs.py 100.00% <100.00%> (ø)
iodata/formats/extxyz.py 86.45% <0.00%> (-7.30%) ⬇️
iodata/utils.py 87.01% <0.00%> (-0.49%) ⬇️
iodata/formats/molden.py 91.30% <0.00%> (-0.03%) ⬇️
iodata/test/test_molden.py 99.24% <0.00%> (-0.01%) ⬇️
... 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 c8e8f10...5c68134. Read the comment docs.

@tovrstra
Copy link
Member

tovrstra commented Sep 3, 2020

(No need to worry about the code coverage failures. These reports are not always accurate but it does add useful comments in the code at times.)

@PaulWAyers
Copy link
Member

@tovrstra @FarnazH can we get this moving? Several people in the group (@BradenDKelly @wilhadams @fwmeng88 Menna, @tczorro) all need this moving forward and are creating their own tools and duplicating work in its absence.

@FanwangM
Copy link
Contributor

FanwangM commented Mar 11, 2021

This is going to be a lovely feature! I like it as I hide my ugly scripts in a safe place.
Thank you!!

Edit: One short question: do we need to use basis_set_exchange to support a non-standard Gaussian basis set or the input module will take care of it? As far as what I have learned from the codes, my guess we have to put the basis set information in the template file.

@tovrstra
Copy link
Member

I was looking into this a few days ago and wanted to go through it in short term, so yep. I'm picking this up shortly. Also other PRs need to be looked at. @fwmeng88 I'll come back to your question after having gone through the diff.

@tovrstra
Copy link
Member

@fwmeng88 You are right any non-standard basis sets would have to be put in the template. This may seem easy at first, but there could be a potential difficulty. Gaussian may not run when given custom basis sets for elements not present in the geometry. If you want to run calculations on a database of molecules which vary in chemical composition, the template would need to be modified for each molecule specifically to just include basis functions for the chemical elements that are present. (This is what I remember from long time ago for an older version of Gaussian, so please double check with the latest version.)

@tovrstra
Copy link
Member

It seems that the free plan of Travis is not exactly working for us. I've tested everything locally and things pass, so I'll merge. Eventually we may have to move away from Travis to get CI up and running again.

@tovrstra tovrstra merged commit 8f1f591 into theochem:master Mar 12, 2021
@FanwangM
Copy link
Contributor

Just checked with the latest version of Gaussian g16/c.01 that we have to specify the basis set exactly only for the atoms that exist in a given molecule. So, things can be tough.

I have opened a new issue related to this with a more detailed explanation. #221
@tovrstra

@PaulWAyers
Copy link
Member

It seems that the free plan of Travis is not exactly working for us. I've tested everything locally and things pass, so I'll merge. Eventually we may have to move away from Travis to get CI up and running again.

Perhaps we should go to GitHub Actions. Procrustes is using GitHub Actions. @fwmeng88 set that up for Procrustes.

@FanwangM
Copy link
Contributor

It seems that the free plan of Travis is not exactly working for us. I've tested everything locally and things pass, so I'll merge. Eventually we may have to move away from Travis to get CI up and running again.

Perhaps we should go to GitHub Actions. Procrustes is using GitHub Actions. @fwmeng88 set that up for Procrustes.

It's not perfect yet but works nicely. https://github.com/theochem/procrustes/blob/master/.github/workflows/testing.yml

@FarnazH FarnazH deleted the write_input branch March 26, 2021 19:58
@FarnazH FarnazH restored the write_input branch March 26, 2021 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants