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

Implementations of is_contiguous and make_contiguous #522

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

SanggyuChong
Copy link

@SanggyuChong SanggyuChong commented Feb 20, 2024

In this PR, we attempt to roll out python-based functions that allow one to check whether or not the values in a TensorBlock and TensorMap are contiguous, and make them contiguous as needed. Tests for the newly implemented operations have also been implemented.

Resolves #265

Contributor (creator of pull-request) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?
  • Issue referenced (for PRs that solve an issue)?

Reviewer checklist

  • CHANGELOG updated with public API or any other important changes?

📚 Documentation preview 📚: https://metatensor--522.org.readthedocs.build/en/522/

Copy link

github-actions bot commented Feb 20, 2024

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

jwa7
jwa7 previously requested changes Feb 21, 2024
Copy link
Member

@jwa7 jwa7 left a comment

Choose a reason for hiding this comment

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

Looking good so far @SanggyuChong , nice job!

I've requested a few changes below. You'll notice on the PR that the build failed - this is because of a linting error. You can use tox -e lint and tox -e format to run the linter and formatter respectively to make sure changes to the code adhere to the style guides in metatensor.

After you've made these changes, let's discuss how to make a metatensor-torch version of this function - probably best done in person.

python/metatensor-operations/tests/contiguous.py Outdated Show resolved Hide resolved
python/metatensor-operations/tests/contiguous.py Outdated Show resolved Hide resolved
python/metatensor-operations/tests/contiguous.py Outdated Show resolved Hide resolved
@jwa7
Copy link
Member

jwa7 commented Feb 21, 2024

Also, if you look at the documentation build here: https://metatensor--522.org.readthedocs.build/en/522/ and navigate to the operations you'll see that these operations aren't yet there. In general 'documentation' in the checklist at the top of this PR covers all of comments, docstrings, API docs, and tutorials - where necessary and appropriate. In this case we'll need an API reference but will have to manually do that - we can discuss this too in the next step!

@jwa7 jwa7 dismissed their stale review April 3, 2024 15:56

I am now taking over the PR

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.

equistore.contiguous operation, making all the data contiguous
2 participants