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
mwfn load_one #198
Conversation
Test files include 0, 1 and 2 mwfn types. The original fchk were taken from iodata/test/data and converted with Multiwfn3.7
I'll fix some minor issues to make the tests pass before review. |
There was a problem hiding this 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
.
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.
61c6b91
to
3cb3e43
Compare
There was a problem hiding this 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!
ecd07db
to
a006bfb
Compare
f69f7b2
to
fe1f842
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@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. |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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. |
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. |
@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. |
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