Skip to content

Conversation

@michaelfortunato
Copy link
Contributor

@michaelfortunato michaelfortunato commented Apr 23, 2025

This commit is the first part of a series of pull requests that we will make to PyTorch Geometric in order add MeshCNN! MeshCNN is a complex and important paper. As such its integration into PyG will be via a series of self-contained PRs. The proposed plan to integrate MeshCNN into PyTorch Geometric is posted as a new comment on the original issue: #283. The full PDF is also downloadable here: https://github.com/user-attachments/files/19872710/MeshCNN-PyG.pdf.

Background

It's important to recognize that the original attempt to integrate MeshCNN into PyG, PR #2293, is quite comprehensive and complete. The code we put here for MeshCNN will may very well look like the original PR. The main difference is that we will split up the pull requests into a series of smaller ones.

This is Part 1 of a comprehensive plan to add MeshCNN into PyTorch Geometric and implements the convolutional layer of MeshCNN. Please see #283 for the comprehensive plan. Please add all comments and feedback related to the entire plan or higher level ideas on the original issue #283. I would love critical feedback on the plan and am happy to completely throw it away or change it based on the feedback I get.

PR's Design

A good place to fully-understand MeshCNNConv's design is by rendering the docs for torch.nn.conv.MeshCNNConv and reading it.

The most important aspect of this pull request is ordering we require on the edge_index (edge adjacency). This constraint is documented in the class (torch.nn.conv.MeshCNNConv).

My questions are:

  1. Should I write the core logic of MeshCNN's (non-permutation invariant) convolution as a new Aggregate module?
  2. MeshCNN's convolution can actually be expressed in terms of the framework of torch_geometric.nn.conv.RGCNConv, where in MeshCNN there are 4 categories. Should we sub-class that instead?
  3. Should we implement batch normalization as a performance optimization here?

I have thought about these questions a lot, and if any of these questions are "yes", my rebuttals to them are

  1. MeshCNN's aggregation is not generalizable enough to warrant the creation of a new aggregation file. That would introduce two new files to implement MeshCNNConv, that is torch.nn.aggr.MeshCNNConv, and the smaller wrapper torch.nn.conv.MeshCNNConv. That being said, moving to torch.nn.aggr.MeshCNNConv is easy to do.
  2. While MeshCNN can be expressed in terms of torch_geometric.nn.conv.RGCNConv, it ties us to any performance characteristics etc that this torch_geometric.nn.conv.RGCNConv may have. I think it is wrong to couple us to this framework, even if conceptually it is important and interesting to recognize MeshCNNConv as a case of torch_geometric.nn.conv.RGCNConv.
  3. This can come as a future and enhancement PR after MeshCNN is more situated into PyG. Adding this complexity now obscures the main and lasting design decisions around our implementation of MeshCNN. These first PR's should get the core functionality out in a way that is straightforward, which will increase the odds that our design decisions now can be easily extended to accommodate future enhancements.

Besides that, this is a small PR, most of the lines being documentation.

Thanks. I am a student in Chicago working independently on this project. As of this week, I am working on this project full-time.

Important

I am always reachable directly via instant message or video calling in PyG's slack. My display name is @Michael Fortunato and my account link is https://torchgeometricco.slack.com/team/U08AMAGE318. Please feel free to message or call me directly, at anytime, with questions or comments. I am also always happy to meet if that helps get things moving.

Thanks so much. Once again, for background on the entire plan please see my comments on
the original issue #283 along with the attached PDF of the plan. To get started with this PR, I recommend viewing the rendered class documentation for torch.nn.conv.MeshCNNConv: https://pytorch-geometric--10222.org.readthedocs.build/en/10222/generated/torch_geometric.nn.conv.MeshCNNConv.html.

Best,
Michael

Slack: @Michael Fortunato on Slack
Original Issue Along With Plan Summary: #283
Full Proposal: MeshCNN-PyG.pdf

This commit is the first part of a series of pull requests that we will
make to PyTorch Geometric in order add MeshCNN! MeshCNN is a complex and
important paper. As such its integration into PyG will be via a series
of self-contained PRs.

# Background

The full plan for MeshCNN is posted in the original issue here: pyg-team#283
In addition, it's important to recognize that the first pyg-team#2293 is quite
comprehensive and complete. The code we put here for MeshCNN will probably
look very much like the original PR made pyg-team#2293. The main, difference is that
we will split up the pull requests into a series of smaller ones.

Again, this is part 1 of a comprehensive plan to add MeshCNN into
PyTorch Geometric. Please see pyg-team#283
<pyg-team#283>
for the comprehensive plan. It is important to understand the paper to an
extent,
*a PDF is attached to the
[issue](pyg-team#283)
with a comprehensive/self-contained explanation of the paper and
how I plan to implement it.*
Please add all comments and feedback
related to the entire plan or higher level ideas.
I would love critical feedback on the plan and am happy to completely
throw it away or change it based on the feedback I get.

To _briefly_ restate the background here (again please see
[pyg-team#238](pyg-team#283),
MeshCNN is a classifier on triangular meshes. MeshCNN has 5 layers,
a triangular mesh to feature layer, a convolutional layer, a pooling
layer, an unpooling layers and a final "normalization" layer (to ensure a
MeshCNN is a valid probability mass function).

This pull request is the first step of the plan,
and implements the convolutional layer of MeshCNN.

# PR's Design

A good place to fully-understand `MeshCNNConv`'s design is by rendering
the docs for `torch.nn.conv.MeshCNNConv` and reading it.

The most important aspect of this pull request is ordering
we require on the `edge_index` (edge adjacency). This constraint is
documented in the class (`torch.nn.conv.MeshCNNConv`).

My questions are:

1. Should I write the core logic of MeshCNN's (non-permutation
   invariant) convolution as a new `Aggregate` module?
2. MeshCNN's convolution can actually be expressed in terms of the framework of
  `torch_geometric.nn.conv.RGCNConv`, where in MeshCNN there are 4
  categories. Should we sub-class that instead?
3. Should we implement batch normalization as a performance optimization
   here?

I have thought about these questions a lot, and if
any of these questions are "yes", my rebuttals to them are

1. MeshCNN's aggregation is not generalizable enough to warrant the
   creation of a new aggregation file. That would introduce two new
   files to implement `MeshCNNConv`, that is
   `torch.nn.aggr.MeshCNNConv`, and the smaller wrapper
   `torch.nn.conv.MeshCNNConv`. That being said, moving to
   `torch.nn.aggr.MeshCNNConv` is easy to do.
2. While MeshCNN can be expressed in terms of
   `torch_geometric.nn.conv.RGCNConv`, it ties us to any performance
   characteristics etc that this `torch_geometric.nn.conv.RGCNConv` may
   have. I think it is wrong to couple us to this framework, even if
   conceptually it is important and interesting to recognize MeshCNNConv
   as a case of `torch_geometric.nn.conv.RGCNConv`.
3. This can come as a future and enhancement PR after MeshCNN is more
   situated into PyG. Adding this complexity now obscures the main and
   lasting design decisions around our implementation of MeshCNN. These
   first PR's should get the core functionality out in a way that is
   straightforward, which will increase the odds that our design
   decisions now can be easily extended to accommodate future
   enhancements.

Besides that, this is a small PR, most of the lines being documentation.

Thanks. _I am a student in Chicago working independently on this project.
As of this week, I am working on this project full-time._

I am *always* reachable directly via instant message or video calling
in PyG's slack. My display name is @michael Fortunato
and my account link is https://torchgeometricco.slack.com/team/U08AMAGE318.
Please feel free to message or call me directly, at anytime, with
questions or comments. I am also always happy to meet if that helps get
things moving.

Thanks so much. Once again, for background on the entire plan please see my
comments on
the [original issue](pyg-team#283)
along with the attached PDF of the plan. To get started with this PR,
I recommend viewing the rendered class documentation for
`torch.nn.conv.MeshCNNConv`. If there is a way for me to self host this
documentation, I will post an URL later today to it so you do not have
to clone my branch and render it yourself.

Best,
Michael

@michael Fortunato on
[Slack](https://torchgeometricco.slack.com/team/U08AMAGE318)
@rusty1s
Copy link
Member

rusty1s commented Jun 3, 2025

Very in-depth documentation, love it :) Thank you so much.

@rusty1s rusty1s enabled auto-merge (squash) June 3, 2025 11:23
@rusty1s rusty1s merged commit 05827c8 into pyg-team:master Jun 3, 2025
16 of 17 checks passed
@codecov
Copy link

codecov bot commented Jun 3, 2025

Codecov Report

❌ Patch coverage is 91.11111% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.81%. Comparing base (c211214) to head (4b865c9).
⚠️ Report is 94 commits behind head on master.

Files with missing lines Patch % Lines
torch_geometric/nn/conv/meshcnn_conv.py 90.90% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10222      +/-   ##
==========================================
- Coverage   86.11%   85.81%   -0.30%     
==========================================
  Files         496      498       +2     
  Lines       33655    34128     +473     
==========================================
+ Hits        28981    29286     +305     
- Misses       4674     4842     +168     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

chrisn-pik pushed a commit to chrisn-pik/pytorch_geometric that referenced this pull request Jun 30, 2025
This commit is the first part of a series of pull requests that we will
make to PyTorch Geometric in order add MeshCNN! MeshCNN is a complex and
important paper. As such its integration into PyG will be via a series
of self-contained PRs. The proposed plan to integrate MeshCNN into
PyTorch Geometric is posted as a new comment on the original issue:
pyg-team#283. The full PDF is also downloadable here:
https://github.com/user-attachments/files/19872710/MeshCNN-PyG.pdf.

# Background

It's important to recognize that the original attempt to integrate
MeshCNN into PyG, PR pyg-team#2293, is quite comprehensive and complete. The
code we put here for MeshCNN will may very well look like the original
PR. The main difference is that we will split up the pull requests into
a series of smaller ones.

This is Part 1 of a comprehensive plan to add MeshCNN into PyTorch
Geometric and implements the convolutional layer of MeshCNN. Please see
pyg-team#283 for the comprehensive plan. Please add all comments and feedback
related to the entire plan or higher level ideas on the original issue
pyg-team#283. I would love critical feedback on the plan and am happy to
completely throw it away or change it based on the feedback I get.

# PR's Design

A good place to fully-understand `MeshCNNConv`'s design is by rendering
the docs for `torch.nn.conv.MeshCNNConv` and reading it.

The most important aspect of this pull request is ordering we require on
the `edge_index` (edge adjacency). This constraint is documented in the
class (`torch.nn.conv.MeshCNNConv`).

My questions are:

1. Should I write the core logic of MeshCNN's (non-permutation
invariant) convolution as a new `Aggregate` module?
2. MeshCNN's convolution can actually be expressed in terms of the
framework of `torch_geometric.nn.conv.RGCNConv`, where in MeshCNN there
are 4 categories. Should we sub-class that instead?
3. Should we implement batch normalization as a performance optimization
here?

I have thought about these questions a lot, and if any of these
questions are "yes", my rebuttals to them are

1. MeshCNN's aggregation is not generalizable enough to warrant the
creation of a new aggregation file. That would introduce two new files
to implement `MeshCNNConv`, that is `torch.nn.aggr.MeshCNNConv`, and the
smaller wrapper `torch.nn.conv.MeshCNNConv`. That being said, moving to
`torch.nn.aggr.MeshCNNConv` is easy to do.
2. While MeshCNN can be expressed in terms of
`torch_geometric.nn.conv.RGCNConv`, it ties us to any performance
characteristics etc that this `torch_geometric.nn.conv.RGCNConv` may
have. I think it is wrong to couple us to this framework, even if
conceptually it is important and interesting to recognize MeshCNNConv as
a case of `torch_geometric.nn.conv.RGCNConv`.
3. This can come as a future and enhancement PR after MeshCNN is more
situated into PyG. Adding this complexity now obscures the main and
lasting design decisions around our implementation of MeshCNN. These
first PR's should get the core functionality out in a way that is
straightforward, which will increase the odds that our design decisions
now can be easily extended to accommodate future enhancements.

Besides that, this is a small PR, most of the lines being documentation.

Thanks. _I am a student in Chicago working independently on this
project._ **As of this week, I am working on this project full-time.**

> [!IMPORTANT] 
> I am always reachable directly via instant message or video calling in
PyG's slack. My display name is `@Michael Fortunato` and my account link
is https://torchgeometricco.slack.com/team/U08AMAGE318. Please feel free
to message or call me directly, at anytime, with questions or comments.
I am also always happy to meet if that helps get things moving.

Thanks so much. Once again, for background on the entire plan please see
my comments on
the original issue pyg-team#283 along with the attached PDF of the plan. To get
started with this PR, I recommend viewing the rendered class
documentation for `torch.nn.conv.MeshCNNConv`:
https://pytorch-geometric--10222.org.readthedocs.build/en/10222/generated/torch_geometric.nn.conv.MeshCNNConv.html.



Best,
Michael

Slack: `@Michael Fortunato` on
[Slack](https://torchgeometricco.slack.com/team/U08AMAGE318)
Original Issue Along With Plan Summary: pyg-team#283
Full Proposal:
[MeshCNN-PyG.pdf](https://github.com/user-attachments/files/19873871/MeshCNN-PyG.pdf)

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Matthias Fey <[email protected]>
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.

2 participants