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

Remove internal metrics in favor of torchmetrics #4287

Merged
merged 16 commits into from
Mar 21, 2022
Merged

Remove internal metrics in favor of torchmetrics #4287

merged 16 commits into from
Mar 21, 2022

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Mar 17, 2022

This PR is resolving #4283

Still some open questions/tasks:

  • shall it be hard removal or with deprecation warning to not breaking user experience (as it is now in this draft)
  • shall be TorchMetrics install or full_install dependency

@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #4287 (91ce15e) into master (a581be6) will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4287      +/-   ##
==========================================
- Coverage   82.47%   82.28%   -0.20%     
==========================================
  Files         311      310       -1     
  Lines       15941    15877      -64     
==========================================
- Hits        13148    13065      -83     
- Misses       2793     2812      +19     
Impacted Files Coverage Δ
torch_geometric/utils/__init__.py 100.00% <ø> (ø)
torch_geometric/nn/conv/utils/typing.py 81.25% <0.00%> (-17.50%) ⬇️
torch_geometric/io/tu.py 93.58% <0.00%> (-2.57%) ⬇️
torch_geometric/graphgym/utils/io.py 78.04% <0.00%> (-2.44%) ⬇️
torch_geometric/nn/models/mlp.py 98.41% <0.00%> (-1.59%) ⬇️
torch_geometric/transforms/gdc.py 77.94% <0.00%> (-1.03%) ⬇️
torch_geometric/nn/conv/rgat_conv.py 83.76% <0.00%> (-0.53%) ⬇️
torch_geometric/graphgym/config.py 94.52% <0.00%> (-0.50%) ⬇️
torch_geometric/graphgym/utils/comp_budget.py 15.51% <0.00%> (+0.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a581be6...91ce15e. Read the comment docs.

@Borda Borda marked this pull request as ready for review March 17, 2022 23:56
@Borda
Copy link
Contributor Author

Borda commented Mar 18, 2022

@rusty1s seems that this PR a slightly lowering the code coverage but it is quite understandable...

@Borda
Copy link
Contributor Author

Borda commented Mar 18, 2022

@rusty1s may I get your opinion on this suggestion/change as the PR description: 🐰

  • hard removal or with deprecation warning to not breaking user experience (as it is now in this draft)
  • TorchMetrics install or full_install dependency

@rusty1s
Copy link
Member

rusty1s commented Mar 18, 2022

@Borda Yes, sorry for the delay :)

  • I think torchmetrics should be a dependency of the full_install.
  • I'm fine with a hard removal TBH since I doubt that these are functions that most PyG users are actively using. If we go for a deprecation warning, is there any chance we can make use of the in-house deprecated warning rather than relying on the external dependency?

@rusty1s rusty1s self-requested a review March 18, 2022 10:01
@Borda
Copy link
Contributor Author

Borda commented Mar 18, 2022

I'm fine with a hard removal TBH since I doubt that these are functions that most PyG users are actively using.

this is something I also do not know that is why I started wth soft removal and can be dropped completely in net release... but happy to follow any your suggestion :)

  • If we go for a deprecation warning, is there any chance we can make use of the in-house deprecated warning rather than relying on the external dependency?

sure I was using a deprecation package I know, but happy to use any according to your suggestion :)

@rusty1s
Copy link
Member

rusty1s commented Mar 18, 2022

Let's go for hard removal :)

@Borda
Copy link
Contributor Author

Borda commented Mar 18, 2022

Let's go for hard removal :)

Done 👍

examples/dgcnn_segmentation.py Outdated Show resolved Hide resolved
examples/dgcnn_segmentation.py Outdated Show resolved Hide resolved
examples/point_transformer_segmentation.py Outdated Show resolved Hide resolved
examples/pointnet2_segmentation.py Outdated Show resolved Hide resolved
Borda and others added 3 commits March 21, 2022 10:58
Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>
@rusty1s rusty1s changed the title Deprecate internal metrics in favor of TorchMetrics Remove internal metrics in favor of torchmetrics Mar 21, 2022
@rusty1s rusty1s merged commit b0d9e75 into pyg-team:master Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using TochMetrics for code simplification
3 participants