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

Add serialization for Topology #567

Merged

Conversation

umesh-timalsina
Copy link
Member

@umesh-timalsina umesh-timalsina commented Jul 7, 2021

This PR attempts to implement a full serialization for topology. Two methods are available:

Topology.save(filename, overwrite=False, **kwargs)
Topology.load(cls, filename) 

@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #567 (da6cf23) into master (00f329c) will increase coverage by 0.19%.
The diff coverage is 93.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
+ Coverage   90.29%   90.49%   +0.19%     
==========================================
  Files          54       56       +2     
  Lines        4205     4408     +203     
==========================================
+ Hits         3797     3989     +192     
- Misses        408      419      +11     
Impacted Files Coverage Δ
gmso/core/pairpotential_type.py 86.95% <57.14%> (-3.05%) ⬇️
gmso/formats/json.py 93.28% <93.28%> (ø)
gmso/abc/gmso_base.py 97.33% <100.00%> (+0.15%) ⬆️
gmso/core/box.py 98.79% <100.00%> (+0.04%) ⬆️
gmso/core/subtopology.py 92.45% <100.00%> (+0.78%) ⬆️
gmso/core/topology.py 96.35% <100.00%> (+0.13%) ⬆️
gmso/formats/__init__.py 100.00% <100.00%> (ø)
gmso/formats/formats_registry.py 100.00% <100.00%> (ø)

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 00f329c...da6cf23. Read the comment docs.

@umesh-timalsina umesh-timalsina requested a review from a team July 13, 2021 17:34
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 13, 2021

This pull request introduces 1 alert when merging f26edf9 into 538049e - view on LGTM.com

new alerts:

  • 1 for Unused import

@umesh-timalsina
Copy link
Member Author

umesh-timalsina commented Jul 13, 2021

This PR also brings in the concept of Registries to save/load files. Only the json methods are currently using the registries but can be extended to other formats as well.

@umesh-timalsina
Copy link
Member Author

The following gist contains simple test cases.

@justinGilmer
Copy link
Collaborator

Just tried this out but one thing i noticed is that the potential types are not being brought over in the json files. Is that expected?

an example:

from mbuild.lib.molecules import Ethane
from gmso.external import from_parmed
import foyer

oplsaa = foyer.forcefields.load_OPLSAA()
eth_typed = oplsaa.apply(Ethane())
topology_typed = from_parmed(topology_typed, refer_types=True)
topology_typed.save("eth.json", overwrite=True, indent=4)

@umesh-timalsina
Copy link
Member Author

Yeah this is expected. You have to explicitly specify types=True while saving it.

@justinGilmer
Copy link
Collaborator

Very true! I forgot about that flag! Working well for me!

justinGilmer added a commit to justinGilmer/gmso that referenced this pull request Jul 27, 2021
After PR mosdef-hub#567, which brought in the addition of concept of Registries to
save/load files; Only the json methods are currently using the registries but can be extended to other formats as well.

This PR introduces the overhaul to the `xyz` file format, one of the
simpler cases. So far, this seems to have worked quite well.
Copy link
Collaborator

@justinGilmer justinGilmer left a comment

Choose a reason for hiding this comment

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

New changes look good. When all checks pass LGTM!

Copy link
Member

@daico007 daico007 left a comment

Choose a reason for hiding this comment

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

Tested it out locally, LGTM!

@justinGilmer justinGilmer merged commit acdbd44 into mosdef-hub:master Jul 27, 2021
@umesh-timalsina umesh-timalsina deleted the 552-topology-serialization branch July 27, 2021 17:56
justinGilmer added a commit to justinGilmer/gmso that referenced this pull request Aug 4, 2021
After PR mosdef-hub#567, which brought in the addition of concept of Registries to
save/load files; Only the json methods are currently using the registries but can be extended to other formats as well.

This PR introduces the overhaul to the `xyz` file format, one of the
simpler cases. So far, this seems to have worked quite well.
daico007 added a commit that referenced this pull request Nov 24, 2021
* Convert xyz file I/O to use the load/save methods

After PR #567, which brought in the addition of concept of Registries to
save/load files; Only the json methods are currently using the registries but can be extended to other formats as well.

This PR introduces the overhaul to the `xyz` file format, one of the
simpler cases. So far, this seems to have worked quite well.

* Overhaul write_gro and read_gro to use the new registry system.

* Update top, gsd, mcf, lammps tests and writer/readers

* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci

* Use deepcopy of the topology when modifying the position for gro files

* Support stacking loads_as, saves_as decorators for multiple file extensions

* Use cheaper deepcopy alternative, simplify decorator for multiple file extensions

Co-authored-by: Co Quach <[email protected]>
Co-authored-by: Umesh Timalsina <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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