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

Add coverage for Python torch bindings #234

Merged
merged 1 commit into from
Oct 18, 2023
Merged

Add coverage for Python torch bindings #234

merged 1 commit into from
Oct 18, 2023

Conversation

PicoCentauri
Copy link
Contributor

@PicoCentauri PicoCentauri commented Sep 21, 2023 •
edited by github-actions bot
Loading

Using the same logic and structure as metatensor/metatensor#381.

📚 Documentation preview 📚: https://rascaline--234.org.readthedocs.build/en/234/

@github-actions
Copy link

github-actions bot commented Sep 21, 2023 •
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

@PicoCentauri PicoCentauri force-pushed the torch-coverage branch 2 times, most recently from d8930ac to ae4cb69 Compare September 22, 2023 06:15
@Luthaf
Copy link
Member

Luthaf commented Sep 22, 2023

It looks like the coverage is very confused... it is reporting empty lines as not-covered: https://app.codecov.io/github/Luthaf/rascaline/commit/ae4cb69ff05f267a229737158a11581baafc7e56/blob/python/rascaline-torch/rascaline/torch/__init__.py, just above _load_library()

@PicoCentauri PicoCentauri force-pushed the torch-coverage branch 5 times, most recently from 02599ac to df9ae98 Compare October 11, 2023 13:41
Luthaf
Luthaf reviewed Oct 11, 2023
View reviewed changes
Copy link
Member

@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.

looks good overall!

.github/workflows/coverage.yml Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@PicoCentauri PicoCentauri requested a review from Luthaf October 16, 2023 16:29
@PicoCentauri
Copy link
Contributor Author

I implemented the suggestion and keep the structure similar to the one in metatensor. I already found some further simplifications but this is something for the next CI cleanup 😅

@Luthaf
Copy link
Member

Luthaf commented Oct 18, 2023

Seems that codecov is no longer adding a commit check with the overall coverage, we'll have to look into this. But this PR should be fine!

@Luthaf Luthaf merged commit 7384f7a into master Oct 18, 2023
24 checks passed
@Luthaf Luthaf deleted the torch-coverage branch October 18, 2023 11:23
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
@PicoCentauri @Luthaf