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

Use torch.det to calculate volumes #130

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

tsihyoung
Copy link
Contributor

Summary

torch.cross without dim is deprecated.

Todos

I'm not sure whether we need a torch.abs here.

`torch.cross` without `dim` is deprecated.
Copy link
Collaborator

@janosh janosh left a comment

Choose a reason for hiding this comment

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

torch.det was the last tensor op in chgnet lacking MPS support which is why we refactored to torch.dot(lattice[0], torch.cross(lattice[1], lattice[2])) to support MPS earlier. based on tests passing on macos-14, looks like torch.det is now implemented in MPS so happy to merge.

@janosh janosh merged commit 5a25b2c into CederGroupHub:main Feb 27, 2024
7 checks passed
@tsihyoung tsihyoung deleted the volume_by_det branch February 27, 2024 14:00
@tsihyoung
Copy link
Contributor Author

torch.det was the last tensor op in chgnet lacking MPS support which is why we refactored to torch.dot(lattice[0], torch.cross(lattice[1], lattice[2])) to support MPS earlier. based on tests passing on macos-14, looks like torch.det is now implemented in MPS so happy to merge.

I tested torch.det on my Apple Silicon machine with pytorch == 2.2.1. Unfortunately, it still complains thatNotImplementedError: The operator 'aten::_linalg_det.result' is not currently implemented for the MPS device.

Currently, maybe we should change the code to torch.dot(lattice[0], torch.cross(lattice[1], lattice[2], dim=-1)).

@janosh
Copy link
Collaborator

janosh commented Feb 27, 2024

interesting, that means the tests on macos-14 aren't actually using MPS. wonder why...

edit: CI logs show CHGNet will run on cpu so indeed MPS is untested

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