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.

Sign up for GitHub

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

Jump to bottom

metatensor interface #158

Merged
merged 9 commits into from
Jan 11, 2025
Merged

metatensor interface #158

merged 9 commits into from
Jan 11, 2025
+1,245 −367

Conversation

frostedoyster
Copy link
Collaborator

A metatensor interface for sphericart

@frostedoyster frostedoyster requested a review from Luthaf January 6, 2025 14:59
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 6, 2025 •
edited
Loading

Here is a pre-built version of the code in this pull request: wheels.zip, you can install it locally by unzipping wheels.zip and using pip to install the file matching your system

Luthaf
Luthaf reviewed Jan 6, 2025
View reviewed changes
Copy link
Contributor

@Luthaf Luthaf 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 looks good, but this might be worth a bit more documentation, both adding a link to metatensor in the class docstring and putting some reference to these new classes in the main documentation.

@frostedoyster frostedoyster force-pushed the metatensor branch 4 times, most recently from 793e32e to a9685fe Compare January 6, 2025 21:43
@frostedoyster
Copy link
Collaborator Author

Unfortunately it wasn't possible to share code between the two interfaces because sphericart-torch doesn't depend on sphericart (and I think it should stay this way)

@frostedoyster frostedoyster marked this pull request as ready for review January 7, 2025 07:48
Luthaf
Luthaf reviewed Jan 7, 2025
View reviewed changes
examples/metatensor/example.py
blocks=[
TensorBlock(
values=np.random.rand(n_samples, 3, 1),
samples=Labels(
Copy link
Contributor

Choose a reason for hiding this comment

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

should we indicate somewhere (maybe here and in the docstring?) that the output will have the same samples as the input?

@Luthaf Luthaf mentioned this pull request Jan 9, 2025
Open
@frostedoyster frostedoyster merged commit 440ea60 into main Jan 11, 2025
26 checks passed
@frostedoyster frostedoyster deleted the metatensor branch January 11, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Luthaf Luthaf Luthaf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@frostedoyster @Luthaf