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

[WIP] Riper calculations + minor modifications on testing + unit tests and integration tests #14

Merged
merged 17 commits into from
Feb 11, 2022

Conversation

davidwaroquiers
Copy link
Member

Adapted MoleculeSystem to handle periodic systems (using Structure object + periodicity as "1D", "2D" or "3D"). The Structure object should have a lattice such that the a vector of the lattice is aligned with the x cartesian axis and the b vector of the lattice is in the xy cartesian plane.
Adapted Gradient object to read periodic system (need to read the control object, as it needs both the $grad datagroup containing the gradients wrt atomic displacements and the $gradlatt datagroup containing the gradients wrt lattice).
Added a first preliminary parsing of Riper log files (integrated into ScfOutput).
Comparison with old parsed objects is now handling None values properly (the reference object is reconstructed and then serialized again to compare to the new serialized object). Before that the reference object was never reconstructed, just read from the json file. When adding a new argument to an object with a default of None, this led to a difference with the new object in the tests.
Missing update of the documentation.

MoleculeSystem can deal with Structure objects.
Additional periodic argument to define periodicity of the Structure.
Extraction of structures from control file (using Gradient).
Parsing of riper outputs (preliminary and experimental).
…/utils.

Updated reference output and parser json files for riper parsing.
Added tests for Ricc2 outputs.
Added unit tests for riper calculations.
@davidwaroquiers
Copy link
Member Author

Need to add a PeriodicData in turbomoleio.output.data and put it in the ScfOutput as well (+ update again all the reference files).

@davidwaroquiers
Copy link
Member Author

Missing the possibility to get PeriodicSystem object from a file where $cell or $lattice also has the "angs" option (so that values are in angstroms instead of the default bohr units).

Moved some of MoleculeSystem and PeriodicSystem objects to BaseSystem.
Correction of the as_dict and from_dict of PeriodicSystem.
Fixed some docstrings.
Fixed checks on periodicity.
@gpetretto
Copy link
Contributor

sorry, I made a bit of a mess with resolved/unresolved conversations. I hope it is fine now.

One additional point: why not having PeriodicSystem in a different module (e.g. periodic.py)? I would not rename molecule.py to contain both for backward compatibility, but it is not really nice there. Not sure if we need an additional module just for the BaseSystem.

@davidwaroquiers
Copy link
Member Author

One additional point: why not having PeriodicSystem in a different module (e.g. periodic.py)? I would not rename molecule.py to contain both for backward compatibility, but it is not really nice there. Not sure if we need an additional module just for the BaseSystem.

Indeed, that might be good. I actually thought about it and having PeriodicSystem in a molecule module doesn't feel right. Putting that in periodic.py would do for me. About BaseSystem, if we indeed move PeriodicSystem to periodic.py, I don't feel importing BaseSystem from molecule.py. Maybe we could still make a base.py, which could hold BaseSystem, but also get_coord_lines and get_mol_and_indices_frozen. Or we could put all of that in utils.py. What do you think ?

@gpetretto
Copy link
Contributor

One additional point: why not having PeriodicSystem in a different module (e.g. periodic.py)? I would not rename molecule.py to contain both for backward compatibility, but it is not really nice there. Not sure if we need an additional module just for the BaseSystem.

Indeed, that might be good. I actually thought about it and having PeriodicSystem in a molecule module doesn't feel right. Putting that in periodic.py would do for me. About BaseSystem, if we indeed move PeriodicSystem to periodic.py, I don't feel importing BaseSystem from molecule.py. Maybe we could still make a base.py, which could hold BaseSystem, but also get_coord_lines and get_mol_and_indices_frozen. Or we could put all of that in utils.py. What do you think ?

seems good to me

@davidwaroquiers
Copy link
Member Author

I've moved BaseSystem, get_coord_lines and get_mol_and_indices_frozen to base.py.

Moved get_coord_lines to base.py.
Moved get_mol_and_indices_frozen to base.py.
Added vesta=True for the lattice (bugfix) where applicable (so that
vectors are properly oriented).
Moved PeriodicSystem's tests to test_periodic.
@davidwaroquiers davidwaroquiers merged commit c636c18 into Matgenix:develop Feb 11, 2022
@davidwaroquiers davidwaroquiers deleted the riper branch March 16, 2022 19:30
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.

3 participants