Skip to content

Add serialization for Topology - #567

Merged
justinGilmer merged 21 commits into
mosdef-hub:masterfrom
umesh-timalsina:552-topology-serialization
Jul 27, 2021
Merged

Add serialization for Topology#567
justinGilmer merged 21 commits into
mosdef-hub:masterfrom
umesh-timalsina:552-topology-serialization

Conversation

@umesh-timalsina

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

Copy link
Copy Markdown
Member

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

codecov Bot commented Jul 7, 2021

Copy link
Copy Markdown

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

lgtm-com Bot commented Jul 13, 2021

Copy link
Copy Markdown
Contributor

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

new alerts:

  • 1 for Unused import

@umesh-timalsina

umesh-timalsina commented Jul 13, 2021

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member Author

The following gist contains simple test cases.

Comment thread gmso/formats/json.py
Comment thread gmso/formats/json.py
Comment thread gmso/tests/test_serialization.py Outdated
@justinGilmer

Copy link
Copy Markdown
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
Copy Markdown
Member Author

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

@justinGilmer

Copy link
Copy Markdown
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.

@justinGilmer justinGilmer left a comment

Copy link
Copy Markdown
Collaborator

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!

@daico007 daico007 left a comment

Copy link
Copy Markdown
Member

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 <43968221+daico007@users.noreply.github.com>
Co-authored-by: Umesh Timalsina <umesh.timalsina@vanderbilt.edu>
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