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

mwfn load_one #198

Merged
merged 33 commits into from Mar 29, 2021
Merged

mwfn load_one #198

merged 33 commits into from Mar 29, 2021

Conversation

BradenDKelly
Copy link
Contributor

Completed load_one function support for *.mwfn file types

Separate future PR's will contain load_many, dump_one, dump_many

Files where added in three locations:
1) tests/data/ had the following files added for testing
ch3_hf_sto3g_fchk_multiwfn3.7.mwfn
ch3_rohf_sto3g_g03_fchk_multiwfn3.7.mwfn
he_spdfgh_virtual_fchk_multiwfn3.7.,wfn
2) tests/ had the following file added for testing:
test_mwfn.py
3) formats/ had the following file added for testing:
mwfn.py

leila-pujal and others added 4 commits August 25, 2020 07:04
Test files include 0, 1 and 2 mwfn types. The original fchk
were taken from iodata/test/data and converted with Multiwfn3.7
@BradenDKelly
Copy link
Contributor Author

@tovrstra @FarnazH Feel free to add yourselves as reviewers since I cannot.

@tovrstra
Copy link
Member

I'll fix some minor issues to make the tests pass before review.

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.

I have mostly minor comments. I'm only a bit puzzled that the code seems to assume Cartesian functions in build_obasis.

iodata/formats/mwfn.py Outdated Show resolved Hide resolved
iodata/formats/mwfn.py Outdated Show resolved Hide resolved
iodata/formats/mwfn.py Outdated Show resolved Hide resolved
iodata/formats/mwfn.py Show resolved Hide resolved
iodata/formats/mwfn.py Outdated Show resolved Hide resolved
iodata/formats/mwfn.py Outdated Show resolved Hide resolved
iodata/test/test_mwfn.py Outdated Show resolved Hide resolved
iodata/test/test_mwfn.py Outdated Show resolved Hide resolved
iodata/test/test_mwfn.py Outdated Show resolved Hide resolved
iodata/test/test_mwfn.py Outdated Show resolved Hide resolved
BradenDKelly and others added 16 commits August 27, 2020 05:00
1. The check for Wfntype, to make sure that wavefunction type is of
   legitimate type, is moved to _load_helper_opener function, because
   the error message raised (in case Wfntype is not known), also shows
   the line number where parsing failed, so it is more helpful to add
   the check right after Wfntype is parsed.
2. The 'mo_kind' value is added to the data dictionary, to avoid the
   need for checking Wfntype again.
1. Checking data['Ncenter'] & data['atnums'] are moved to the functions
   which load them, because it is more helpful to raise the error where
   the information is read, as the error message also shows the line
   number causing error.
2. Checking data['shell_types'], data['shell_contraction_degrees'], and
   data['shell_centers'] length are removed, because that would be true
   by construction.
This allows for better checking the title of the section being parsed,
and makign sure primitive exponents & coefficients are correctly assigned.
Unlike other formats (like WFN), itt is pretty simple to build
MolecularBasis from parsed MWFN data, so the _build_obasis func is
removed. This makes the code more clear and easier to read.
1) The raised message would be more informative that the assert error
2) The `assert len(section) == nprim` is redundant, because the while
   loop gaurantees that it would have the same number of elements as
   nprim.
Most of the items in the extra dictionary were already existing amon
IOData attributes, so they were removed.
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.

I have a few very minor comments, which should be easy to fix. Thanks for all the nice work!

iodata/formats/mwfn.py Outdated Show resolved Hide resolved
iodata/formats/mwfn.py Outdated Show resolved Hide resolved
iodata/formats/mwfn.py Outdated Show resolved Hide resolved
iodata/formats/mwfn.py Outdated Show resolved Hide resolved
iodata/test/test_mwfn.py Outdated Show resolved Hide resolved
iodata/test/test_mwfn.py Outdated Show resolved Hide resolved
iodata/test/test_mwfn.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 1, 2020

Codecov Report

Merging #198 (7d081f9) into master (ced78e6) will decrease coverage by 0.14%.
The diff coverage is 89.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
- Coverage   95.07%   94.93%   -0.15%     
==========================================
  Files          72       74       +2     
  Lines        7374     7676     +302     
  Branches      986     1020      +34     
==========================================
+ Hits         7011     7287     +276     
- Misses        175      188      +13     
- Partials      188      201      +13     
Impacted Files Coverage Δ
iodata/orbitals.py 72.04% <ø> (ø)
iodata/formats/mwfn.py 81.02% <81.02%> (ø)
iodata/test/test_mwfn.py 100.00% <100.00%> (ø)
iodata/api.py 85.91% <0.00%> (-0.58%) ⬇️
iodata/formats/mol2.py 89.71% <0.00%> (-0.19%) ⬇️
iodata/formats/fchk.py 90.17% <0.00%> (-0.06%) ⬇️
iodata/overlap.py 100.00% <0.00%> (ø)
iodata/periodic.py 100.00% <0.00%> (ø)
iodata/formats/pdb.py 100.00% <0.00%> (ø)
iodata/inputs/orca.py 100.00% <0.00%> (ø)
... and 8 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 ced78e6...7d081f9. Read the comment docs.

@tovrstra
Copy link
Member

@BradenDKelly I've merged master back into this branch to run it through the rebooted CI. I'll go through it later. This has been open for too long.

@tovrstra
Copy link
Member

@BradenDKelly @FarnazH I fixed a few minor issues. As far as I'm concerned, this is ready to be merged.

@BradenDKelly You may want to double check my changes in the initialization of the Orbitals object. I believe there was a mistake with the norba and norbb arguments. I've updated their docstrings to clarify their meaning.

tovrstra
tovrstra previously approved these changes Mar 22, 2021
@FarnazH FarnazH self-requested a review March 28, 2021 23:55
Copy link
Member

@FarnazH FarnazH 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 fixing the mistake in initializing MolecularOrbitals @tovrstra. I think the new validators added in issue #245 would have caught it, yes?

I took a last look and removed redundant properties stored in the extra dictionary. I am good with merging this PR.

@tovrstra
Copy link
Member

Thanks @FarnazH. I'm good with merging as it is now. @BradenDKelly If you notice something after merging, feel free to open another PR. In any case, thanks everyone for the nice work.

@tovrstra tovrstra merged commit deba3d7 into theochem:master Mar 29, 2021
@BradenDKelly
Copy link
Contributor Author

Thanks @tovrstra and @FarnazH. I have a prototype of a dump_one I will whittle away at. Also, somewhat of a tangent, but it does not appear that we have a *.cif reader yet do we? Thinking selfishly of future Braden, I think if we do not, I will open a PR soon for that and get a load_one, dump_one for *.cif, or at least, a load_one.

@tovrstra
Copy link
Member

Let's open an issue for CIF, where we can collect useful info regarding this format. It is not entirely trivial but certainly useful. The format is fairly complicated, with it's own container format and a specification within that container.

@tovrstra tovrstra deleted the MWFN_LoadOne_New branch March 29, 2021 18:52
@tovrstra tovrstra mentioned this pull request Mar 29, 2021
@PaulWAyers
Copy link
Member

@BradenDKelly if you polish your prototype of dump_one for *.mwfn please make sure you go through the load_one to make sure whatever changes Toon and Farnaz made there filter down....less work for everyone that way.

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

5 participants