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

Added vector output from skew-symmetric tensor #301

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

shenoynikhil
Copy link

Allowing vector output from TensorNet. This will allow the use of EquivariantScalar or EquivariantVectorOutput from TensorNet.

Resolves #297.

@guillemsimeon
Copy link
Collaborator

Thanks a lot! Can you add the equivariance test, please? I understood what you meant in the issue (it just confused me to see them denotes as 'forces' and not specifying further :) )

@shenoynikhil
Copy link
Author

I should be able to add it today. Sorry for the delay.

@guillemsimeon
Copy link
Collaborator

guillemsimeon commented Mar 3, 2024 via email

@shenoynikhil
Copy link
Author

shenoynikhil commented Mar 7, 2024

Can you add the equivariance test, please?

Added an equivariance test.

@guillemsimeon
Copy link
Collaborator

Hey! Sorry for the delay. Revisiting this:

Your approach seems correct @shenoynikhil, however I think I would not proceed by adding a new model that is called equivariant-tensornet. This makes me think, @RaulPPelaez, that we should get rid of the is_equivariant flag, and directly modify the places where the ET is used (yaml example files, for example), and directly include there EquivariantScalar, instead of Scalar, as output model, where it is used. What do you think? I don't know how many things this change would impact, but in any case I think it is worth doing it, the flag does not make much sense at this point...

For me, I would say it is fine if TensorNet defaults to returning x, v instead of x, None. Even though TensorNet uses Scalar (not EquivariantScalar), for energy predictions, I have the impression that nothing would happen. As I said above at some point, v comes from A, which has already been used by learnable parameters to generate x. Using Scalar, no new learnable things would act independently on v, and therefore we should not get an error pointing to not using parameters that are trainable in the forward.

What do you think guys?

@@ -120,6 +124,7 @@ class TensorNet(nn.Module):
(default: :obj:`True`)
check_errors (bool, optional): Whether to check for errors in the distance module.
(default: :obj:`True`)
vector_output (bool, optional): Whether to return
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sentence is cut off mid way it seems

@RaulPPelaez
Copy link
Collaborator

I would not proceed by adding a new model that is called equivariant-tensornet

I agree because it kind of implies that there is a non-equivariant tensornet.
I would be more comfortable with tensornet setting is_equivariant (and thus selecting the Equivariant* output models) if vector_output is set to true. This option could come from train.py as "tensornet-vector-output" maybe.
It is not standard for us to refer to different flavors of a model with different names, but I am not totally against it personally. In this particular case I do not think it is a good idea, imagine I have "vector-tensornet" on one hand and "tensornet-whatever" in another and I would like "vector-tensornet-whatever", there would be no way to get that.

For me, I would say it is fine if TensorNet defaults to returning x, v instead of x, None. Even though TensorNet uses Scalar (not EquivariantScalar), for energy predictions, I have the impression that nothing would happen

I do not like this because even if autograd eats it it is extra unnecessary work. I believe gating the functionality behind an user-provided function or a model name is best in this case.

we should get rid of the is_equivariant flag, and directly modify the places where the ET is used (yaml example files, for example), and directly include there EquivariantScalar, instead of Scalar, as output model, where it is used.

I agree with this, but I believe it is out of the scope of this PR. It would be great if you open an issue so we have it in our list...
For retrocompatibility create_model can simply compare "model_name" with "equivariant-transformer" to decide whether to transform Scalar into EquivariantScalar.

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.

[Feature Request] Vector output from TensorNet
3 participants