Skip to content

support init-frz-model for se_r and se_t#1144

Merged
amcadmus merged 5 commits intodeepmodeling:develfrom
njzjz:init-frz-model-ser-set
Sep 14, 2021
Merged

support init-frz-model for se_r and se_t#1144
amcadmus merged 5 commits intodeepmodeling:develfrom
njzjz:init-frz-model-ser-set

Conversation

@njzjz
Copy link
Member

@njzjz njzjz commented Sep 13, 2021

I just copied several codes from se_a, which can also be applied to se_r and se_t.

I just copied several codes from se_a, which can also be applied to se_r and se_t.
@njzjz njzjz requested review from amcadmus and denghuilu September 13, 2021 23:01
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2021

Codecov Report

Merging #1144 (b77ad4d) into devel (76bc05c) will increase coverage by 0.00%.
The diff coverage is 80.64%.

Impacted file tree graph

@@           Coverage Diff           @@
##            devel    #1144   +/-   ##
=======================================
  Coverage   75.66%   75.66%           
=======================================
  Files          88       89    +1     
  Lines        7034     7048   +14     
=======================================
+ Hits         5322     5333   +11     
- Misses       1712     1715    +3     
Impacted Files Coverage Δ
deepmd/descriptor/se.py 68.42% <68.42%> (ø)
deepmd/descriptor/se_a.py 96.27% <100.00%> (+1.82%) ⬆️
deepmd/descriptor/se_r.py 91.91% <100.00%> (-0.05%) ⬇️
deepmd/descriptor/se_t.py 98.04% <100.00%> (-0.01%) ⬇️
deepmd/utils/graph.py 74.11% <100.00%> (ø)
deepmd/entrypoints/compress.py 77.19% <0.00%> (-2.40%) ⬇️

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 76bc05c...b77ad4d. Read the comment docs.

@amcadmus
Copy link
Member

amcadmus commented Sep 14, 2021

I just copied several codes from se_a, which can also be applied to se_r and se_t.

Are they tested?

Should these methods be put in Descriptor?

@njzjz
Copy link
Member Author

njzjz commented Sep 14, 2021

I just copied several codes from se_a, which can also be applied to se_r and se_t.

Are they tested?

Yes and they work fine.

Should these methods be put in Descriptor?

At least the hybrid descriptor needs specific methods so I prefer not putting in Descriptor.

However, both these descriptors has a same embedding network, so it is possible for them to share some methods (like those in this PR). So I would prefer adding another base class for them.

@njzjz njzjz marked this pull request as draft September 14, 2021 00:44
"""
embedding_net_nodes = {}
embedding_net_pattern = f"filter_type_\d+{suffix}/matrix_\d+_\d+|filter_type_\d+{suffix}/bias_\d+_\d+|filter_type_\d+{suffix}/idt_\d+_\d+|filter_type_all{suffix}/matrix_\d+_\d+|filter_type_all{suffix}/bias_\d+_\d+|filter_type_all{suffix}/idt_\d+_\d"
embedding_net_pattern = f"filter_type_\d+{suffix}/matrix_\d+_\d+|filter_type_\d+{suffix}/bias_\d+_\d+|filter_type_\d+{suffix}/idt_\d+_\d+|filter_type_all{suffix}/matrix_\d+_\d+|filter_type_all{suffix}/matrix_\d+_\d+_\d+|filter_type_all{suffix}/bias_\d+_\d+|filter_type_all{suffix}/bias_\d+_\d+_\d+|filter_type_all{suffix}/idt_\d+_\d"
Copy link
Member Author

Choose a reason for hiding this comment

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

I just added filter_type_all{suffix}/matrix_\d+_\d+_\d+ and filter_type_all{suffix}/bias_\d+_\d+_\d+ to support se_e3. @denghuilu I have two questions:

  1. The pattern is too complex now. Is it possible to use something like filter_type_*/* to get all these things.
  2. Is filter_type_all{suffix}/idt_\d+_\d a typo of filter_type_all{suffix}/idt_\d+_\d+?

Copy link
Member

Choose a reason for hiding this comment

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

I just added filter_type_all{suffix}/matrix_\d+_\d+_\d+ and filter_type_all{suffix}/bias_\d+_\d+_\d+ to support se_e3. @denghuilu I have two questions:

  1. The pattern is too complex now. Is it possible to use something like filter_type_*/* to get all these things.
  2. Is filter_type_all{suffix}/idt_\d+_\d a typo of filter_type_all{suffix}/idt_\d+_\d+?
  1. I think the answer is no, we also have nodes like filter_type_1/add_5, filter_type_1/Tanh_3, filter_type_1/matrix_2_1/read within the graph,

  2. Yes, It's a typo

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I pushed a commit to fix this typo.

@njzjz njzjz requested a review from denghuilu September 14, 2021 01:50
@njzjz njzjz marked this pull request as ready for review September 14, 2021 01:51
@amcadmus amcadmus merged commit 424970e into deepmodeling:devel Sep 14, 2021
@njzjz njzjz deleted the init-frz-model-ser-set branch October 2, 2021 19:41
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