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

Reformat dpa1 #176

Open
wants to merge 5 commits into
base: reformat
Choose a base branch
from
Open

Reformat dpa1 #176

wants to merge 5 commits into from

Conversation

iProzd
Copy link
Collaborator

@iProzd iProzd commented Jan 24, 2024

This PR aims to reformat the dpa1 descriptor in both pytorch and tensorflow repo.

  • Cleanup unused parameters for dpa1 in pytorch version
  • Add serialize and deserialize method
  • Add numpy method to be consistent with pytorch and tensroflow

add serialize and deserialize method for dpa1 descriptor
fix uts;
remove self.networks in NetworkCollection
Copy link
Collaborator

@wanghan-iapcm wanghan-iapcm left a comment

Choose a reason for hiding this comment

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

Overall I like the PR very much. I am keen to see the numpy implementation is simultaneously PRed to the deepmd-kit repo

yy: torch.Tensor
The output.
"""
yy = F.embedding(xx, self.matrix)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the operation of torch.nn.functional.embedding? how would you implement it in numpy?

@@ -3,6 +3,7 @@
import numpy as np
import torch
import torch.nn as nn
import torch.nn.functional as F
Copy link
Collaborator

Choose a reason for hiding this comment

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

very bad short-hand name. change required.

@@ -179,6 +179,219 @@ def deserialize(cls, data:dict)->"MLPLayer":
return obj


class EmbdLayer(nn.Module):
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be implemented via native layer?

yy: torch.Tensor
The output.
"""
yy = F.layer_norm(xx, tuple((self.num_in,)), self.matrix, self.bias, self.eps)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why layer_norm contains trainable parameters?
how would you implement it in numpy? please provide the implementation to deepmd-kit repo

dtype = env.GLOBAL_PT_FLOAT_PRECISION


class TestCaseSingleFrameWithNlist():
Copy link
Collaborator

Choose a reason for hiding this comment

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

better to import from test_se_e2_a.py

@@ -406,7 +406,7 @@ def forward(
nfnl = dmatrix.shape[0]
# pre-allocate a shape to pass jit
xyz_scatter = torch.zeros([nfnl, 4, self.filter_neuron[-1]], dtype=self.prec, device=env.DEVICE)
for ii,ll in enumerate(self.filter_layers.networks):
for ii,ll in enumerate(self.filter_layers._networks):
Copy link
Collaborator

Choose a reason for hiding this comment

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

better providing a method to access the data of a class object.

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.

2 participants