Skip to content

Implement Support for pymatgen.core.Molecule#200

Merged
amcadmus merged 13 commits into
deepmodeling:develfrom
tuoping:supportMolecule
Oct 18, 2021
Merged

Implement Support for pymatgen.core.Molecule#200
amcadmus merged 13 commits into
deepmodeling:develfrom
tuoping:supportMolecule

Conversation

@tuoping

@tuoping tuoping commented Sep 13, 2021

Copy link
Copy Markdown
Collaborator
  • Registered def remove_pbc() in to Format.post("remove_pbc") system.py. And attributed it to PyMatgenMoleculeFormat.from_system() in plugins/pymatgen.py.
  • Moved some operations under def remove_pbc() to pymatgen/molecule.py.
  • Called remove_pbc(data) before calling pymatgen.Molecule(coords = data['coords']), so to guarantee that the molecule doesn't get cut by cell boundaries.

Unittest script: tests/test_pymatgen_molecule.py
Unittest files: tests/pymatgen/*

@codecov-commenter

codecov-commenter commented Sep 13, 2021

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 93.65079% with 4 lines in your changes missing coverage. Please review.

Project coverage is 81.66%. Comparing base (f594787) to head (1e248ce).
Report is 387 commits behind head on devel.

Files with missing lines Patch % Lines
dpdata/plugins/pymatgen.py 84.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            devel     #200      +/-   ##
==========================================
+ Coverage   81.38%   81.66%   +0.28%     
==========================================
  Files          54       55       +1     
  Lines        4592     4690      +98     
==========================================
+ Hits         3737     3830      +93     
- Misses        855      860       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tuoping tuoping requested review from amcadmus and njzjz September 13, 2021 07:04
Comment thread tests/test_pymatgen_molecule.py Outdated
Comment thread dpdata/pymatgen/molecule.py Outdated
@amcadmus amcadmus requested a review from njzjz September 14, 2021 00:20
Comment thread dpdata/plugins/pymatgen.py Outdated
@tuoping tuoping marked this pull request as draft September 14, 2021 08:46
@tuoping tuoping marked this pull request as ready for review September 29, 2021 09:17
@tuoping tuoping requested review from amcadmus and njzjz September 30, 2021 13:13
Comment thread dpdata/pymatgen/molecule.py Outdated
Comment on lines +28 to +41
def remove_pbc(system, protect_layer = 9):
nframes = len(system["coords"])
natoms = len(system['coords'][0])
for ff in range(nframes):
tmpcoord = system['coords'][ff]
cog = np.average(tmpcoord, axis = 0)
dist = tmpcoord - np.tile(cog, [natoms, 1])
max_dist = np.max(np.linalg.norm(dist, axis = 1))
h_cell_size = max_dist + protect_layer
cell_size = h_cell_size * 2
shift = np.array([1,1,1]) * h_cell_size - cog
system['coords'][ff] = system['coords'][ff] + np.tile(shift, [natoms, 1])
system['cells'][ff] = cell_size * np.eye(3)
return system

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.

It may not be a good idea to place remove_pbc here, because this method is not specific to pymatgen/molecule

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I moved "remove_pbc" to system.py. How is it now ?

Comment thread dpdata/system.py
Comment thread dpdata/system.py
Comment on lines -444 to +447
nframes = self.get_nframes()
natoms = self.get_natoms()
assert(protect_layer >= 0), "the protect_layer should be no less than 0"
for ff in range(nframes):
tmpcoord = self.data['coords'][ff]
cog = np.average(tmpcoord, axis = 0)
dist = tmpcoord - np.tile(cog, [natoms, 1])
max_dist = np.max(np.linalg.norm(dist, axis = 1))
h_cell_size = max_dist + protect_layer
cell_size = h_cell_size * 2
shift = np.array([1,1,1]) * h_cell_size - cog
self.data['coords'][ff] = self.data['coords'][ff] + np.tile(shift, [natoms, 1])
self.data['cells'][ff] = cell_size * np.eye(3)
remove_pbc(self.data, protect_layer)

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.

I would recommend reverting the remove_pbc method in dpdata.System

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If this is leaved in class dpdata.System, how do I invoke it from pymatgen/molecule.py?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

where do you need remove_pbc in pymatgen/molecule.py? I do not find it

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sorry for the late reply. Actually, I need remove_pbc in plugins/pymatgen.py. Specifically, in class PyMatgenMoleculeFormat(Format): def to_system(self, ...).

@amcadmus amcadmus merged commit 9b35c81 into deepmodeling:devel Oct 18, 2021
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.

5 participants