Skip to content

implement descriptor plugin; pass params to Descriptor#1118

Merged
amcadmus merged 6 commits intodeepmodeling:develfrom
njzjz:descriptor-plugin
Sep 11, 2021
Merged

implement descriptor plugin; pass params to Descriptor#1118
amcadmus merged 6 commits intodeepmodeling:develfrom
njzjz:descriptor-plugin

Conversation

@njzjz
Copy link
Member

@njzjz njzjz commented Sep 9, 2021

This PR adds plugin systems to descriptors. It's aimed to make deepmd-kit become a framework with any new or old descriptors.

This PR also adds support to pass parameters to Descriptor class, i.e. descript = Descriptor(type="se_e2_a", rcut=6., rcut_smth=0.5, sel=[50]), which is aimed to make it possible to build models with Python instead of command lines.

The next goal is to do the same thing to fittings, models, et al.

Also, I am going to write an example and maybe implement BPNN or other third-party descriptors.

@njzjz njzjz marked this pull request as draft September 9, 2021 18:41
@njzjz njzjz requested review from amcadmus and denghuilu September 9, 2021 19:51
@njzjz njzjz marked this pull request as ready for review September 9, 2021 19:51
@Descriptor.register("se_ar")
class DescrptSeAR (Descriptor):
def __init__ (self, jdata):
def __init__ (self, **jdata):
Copy link
Member

Choose a reason for hiding this comment

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

This change actually introduces a bug, DescrptSeAR because takes dict as input.

DescrptSeAR is replace by DescrptHybrid.

Now deepmd-kit does not accept the descriptor type "se_ar", so we can safely remove it

Copy link
Member Author

@njzjz njzjz Sep 10, 2021

Choose a reason for hiding this comment

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

Ok, I've reverted changes to DescrptSeAR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we directly remove this file?

Copy link
Member

Choose a reason for hiding this comment

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

Yes we can. Remember also remove it UT.

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2021

Codecov Report

Merging #1118 (1e7cbb0) into devel (97be2f5) will increase coverage by 0.19%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##            devel    #1118      +/-   ##
==========================================
+ Coverage   75.46%   75.66%   +0.19%     
==========================================
  Files          88       88              
  Lines        7036     7034       -2     
==========================================
+ Hits         5310     5322      +12     
+ Misses       1726     1712      -14     
Impacted Files Coverage Δ
deepmd/descriptor/hybrid.py 33.73% <11.11%> (-3.11%) ⬇️
deepmd/descriptor/descriptor.py 76.59% <81.25%> (+1.59%) ⬆️
deepmd/__init__.py 82.35% <83.33%> (+0.53%) ⬆️
deepmd/utils/plugin.py 84.37% <84.37%> (ø)
deepmd/descriptor/__init__.py 100.00% <100.00%> (ø)
deepmd/descriptor/se_a.py 94.44% <100.00%> (+0.03%) ⬆️
deepmd/descriptor/se_a_ebd.py 16.32% <100.00%> (+1.74%) ⬆️
deepmd/descriptor/se_a_ef.py 55.31% <100.00%> (+0.23%) ⬆️
deepmd/descriptor/se_r.py 91.95% <100.00%> (-0.94%) ⬇️
deepmd/descriptor/se_t.py 98.05% <100.00%> (+0.02%) ⬆️
... and 9 more

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 97be2f5...1e7cbb0. Read the comment docs.

@amcadmus amcadmus merged commit 90d71ca into deepmodeling:devel Sep 11, 2021
njzjz added a commit to njzjz/deepmd-kit that referenced this pull request Sep 13, 2021
@njzjz njzjz mentioned this pull request Sep 13, 2021
@njzjz njzjz deleted the descriptor-plugin branch September 13, 2021 21:28
amcadmus pushed a commit that referenced this pull request Sep 14, 2021
This fixes a typo in #1118.
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.

4 participants