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

decouple activation function's type from model compression's process in SE_A, now tanh & gelu is both available. #1020

Merged
merged 8 commits into from
Aug 27, 2021

Conversation

liangadam
Copy link
Contributor

I modified 3 pieces of code and wrote a brief unittest 'source/tests/test_tabulate.py' .

Now, when the 'activation_function' is designated as 'tanh' or 'gelu' in descriptor SE_A, model compression can produce a reasonable model.

Moreover, cause the type of activation function is fully decoupled from the process of model compression, now we can add any other function type we need in a very easy way.

HLA and others added 2 commits August 24, 2021 00:00
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2021

Codecov Report

Merging #1020 (b946719) into devel (fd1f371) will decrease coverage by 10.72%.
The diff coverage is n/a.

❗ Current head b946719 differs from pull request most recent head 1c71fee. Consider uploading reports for the commit 1c71fee to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##            devel    #1020       +/-   ##
===========================================
- Coverage   75.01%   64.28%   -10.73%     
===========================================
  Files          86        5       -81     
  Lines        6924       14     -6910     
===========================================
- Hits         5194        9     -5185     
+ Misses       1730        5     -1725     
Impacted Files Coverage Δ
source/op/_gelu.py
deepmd/utils/learning_rate.py
deepmd/infer/__init__.py
deepmd/cluster/slurm.py
deepmd/infer/model_devi.py
source/op/_tabulate_grad.py
deepmd/op/__init__.py
deepmd/infer/deep_polar.py
deepmd/infer/deep_dipole.py
deepmd/model/__init__.py
... and 60 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 fd1f371...1c71fee. Read the comment docs.

Copy link
Member

@njzjz njzjz left a comment

Choose a reason for hiding this comment

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

Also, please notice that tests failed.

deepmd/utils/tabulate.py Outdated Show resolved Hide resolved
deepmd/utils/tabulate.py Outdated Show resolved Hide resolved
@njzjz njzjz requested a review from denghuilu August 23, 2021 20:58
Copy link
Member

@denghuilu denghuilu left a comment

Choose a reason for hiding this comment

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

The code indentation is inconsistent in multiple places, please check it.

deepmd/utils/tabulate.py Outdated Show resolved Hide resolved
source/op/unaggregated_grad.cc Outdated Show resolved Hide resolved
@njzjz
Copy link
Member

njzjz commented Aug 25, 2021

Please revert changes from this commit to tabulate.py... We do not accept style format without actual change for this reason.

Comment on lines 323 to 426
// UnaggregatedDyDxOp<GPUDevice, T>);
// UnaggregatedDyDxOp<GPUDevice, T>);
Copy link
Member

Choose a reason for hiding this comment

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

Plz revert all such format changes! thank you

Comment on lines 155 to 209
y.shape(),
&dy_dx));
y.shape(),
&dy_dx));
Copy link
Member

Choose a reason for hiding this comment

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

Should be reverted.

y.shape(),
&dy2_dx));
y.shape(),
&dy2_dx));

Copy link
Member

Choose a reason for hiding this comment

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

Should be reverted.

Copy link
Member

Choose a reason for hiding this comment

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

plz revert to exactly the same as before

Comment on lines 234 to 300
z.shape(),
&dz_dx));
z.shape(),
&dz_dx));
Copy link
Member

Choose a reason for hiding this comment

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

Should be reverted.

Comment on lines 279 to 349
z.shape(),
&dz2_dx));
z.shape(),
&dz2_dx));
Copy link
Member

Choose a reason for hiding this comment

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

Should be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code mixed tab and space, using them in the same line. I just have normalized them.

Comment on lines +53 to +59
# functype
if activation_fn == ACTIVATION_FN_DICT["tanh"]:
self.functype = 1
elif activation_fn == ACTIVATION_FN_DICT["gelu"]:
self.functype = 2
else:
raise RunTimeError("Unknown actication function type!")
Copy link
Member

Choose a reason for hiding this comment

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

we should support all activation functions in ACTIVATION_FN_DICT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will be done in future work.

Comment on lines 155 to 209
y.shape(),
&dy_dx));
y.shape(),
&dy_dx));
Copy link
Member

Choose a reason for hiding this comment

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

plz revert to exactly the same as before

y.shape(),
&dy2_dx));
y.shape(),
&dy2_dx));

Copy link
Member

Choose a reason for hiding this comment

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

plz revert to exactly the same as before

Comment on lines 234 to 298
z.shape(),
&dz_dx));
z.shape(),
&dz_dx));
Copy link
Member

Choose a reason for hiding this comment

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

plz revert to exactly the same as before

Comment on lines 279 to 346
z.shape(),
&dz2_dx));
z.shape(),
&dz2_dx));
Copy link
Member

Choose a reason for hiding this comment

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

plz revert to exactly the same as before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Tab 4space Tab Tab Tab Tab Tab 4space

@amcadmus amcadmus requested a review from njzjz August 26, 2021 12:57
Copy link
Member

@denghuilu denghuilu left a comment

Choose a reason for hiding this comment

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

nice work @liangadam

@amcadmus amcadmus merged commit 5ab5fa1 into deepmodeling:devel Aug 27, 2021
gzq942560379 pushed a commit to HPC-AI-Team/deepmd-kit that referenced this pull request Sep 2, 2021
…in SE_A, now tanh & gelu is both available. (deepmodeling#1020)

* commit-message: decouple activation function's type from model compression's process in SE_A, now tanh & gelu is both available.

* commit-message: modified code and passed unittest

* commit-message: Format Document

* commit-message :Format revert

* commit-message: format change

* commit-message: Format change

Co-authored-by: HLA <[email protected]>
njzjz pushed a commit to njzjz/deepmd-kit that referenced this pull request Sep 21, 2023
…pmodeling#1020)

python3.10 support: collection.Mapping -> collection.abc.Mapping

repair this: deepmodeling/dpgen#1019
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