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

Add Graph class #403

Open
wants to merge 8 commits into
base: 0.2
Choose a base branch
from
Open

Add Graph class #403

wants to merge 8 commits into from

Conversation

FilippoOlivo
Copy link
Member

@FilippoOlivo FilippoOlivo commented Jan 16, 2025

This PR refers to the issue #400

@dario-coscia
Copy link
Collaborator

@FilippoOlivo I would avoid to write a graph solver, SupervisedSolver should be used

@FilippoOlivo FilippoOlivo force-pushed the 0.2 branch 10 times, most recently from 567f207 to 1867274 Compare January 24, 2025 09:16
@FilippoOlivo FilippoOlivo marked this pull request as ready for review January 24, 2025 12:41
@FilippoOlivo FilippoOlivo added the pr-to-fix Label for PR that needs modification label Jan 24, 2025
@FilippoOlivo FilippoOlivo self-assigned this Jan 24, 2025
@FilippoOlivo FilippoOlivo requested a review from gc031298 January 24, 2025 12:41
@dario-coscia dario-coscia changed the title Add Graph class and Supervised Graph solver Add Graph class Jan 24, 2025
@FilippoOlivo FilippoOlivo force-pushed the 0.2 branch 5 times, most recently from 11a43ec to 290641a Compare January 25, 2025 13:20
@FilippoOlivo FilippoOlivo marked this pull request as draft January 27, 2025 08:32
@FilippoOlivo FilippoOlivo force-pushed the 0.2 branch 3 times, most recently from 7ba1b32 to 238f665 Compare January 27, 2025 15:26
@FilippoOlivo FilippoOlivo marked this pull request as ready for review January 27, 2025 15:26
@FilippoOlivo FilippoOlivo added pr-to-review Label for PR that are ready to been reviewed and removed pr-to-fix Label for PR that needs modification labels Jan 27, 2025
@FilippoOlivo FilippoOlivo marked this pull request as draft January 27, 2025 16:47
@ndem0
Copy link
Member

ndem0 commented Jan 29, 2025

I would say green light on my side!

@dario-coscia
Copy link
Collaborator

@FilippoOlivo there is still the supervised.py file not tracked. Also I remember last time we discussed to divide the classes:

  • A base GraphInterface
  • RadiusGraph
  • KNNGraph
  • Graph
  • TemporalGraph
  • ....

@dario-coscia dario-coscia added pr-to-fix Label for PR that needs modification v0.2 implementation in v0.2 and removed pr-to-review Label for PR that are ready to been reviewed labels Jan 30, 2025
@FilippoOlivo FilippoOlivo force-pushed the 0.2 branch 2 times, most recently from 17962fd to 89685a5 Compare February 3, 2025 14:51
@dario-coscia
Copy link
Collaborator

@FilippoOlivo @gc031298 This refactoring looks very nice!

@FilippoOlivo FilippoOlivo force-pushed the 0.2 branch 2 times, most recently from 67d39b0 to d1058e9 Compare February 4, 2025 10:50
@FilippoOlivo FilippoOlivo marked this pull request as ready for review February 5, 2025 10:16
@FilippoOlivo FilippoOlivo added pr-to-review Label for PR that are ready to been reviewed and removed pr-to-fix Label for PR that needs modification labels Feb 5, 2025
pina/data/dataset.py Show resolved Hide resolved
custom_build_edge_attr=None,
additional_params=None
):
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add a more rich doc

Copy link
Member Author

Choose a reason for hiding this comment

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

Done and pushed. Let me know if now it is good enough

return 1

@staticmethod
def _check_input_consistency(x, pos, edge_index=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add a raise TypeError if the conditions are not met (one for x one for pos and one for edge_index)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also in other function I would add as many raise as possible, while being precise on the error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

I add error handling in the function

pina/graph.py Outdated
x, pos, edge_index = self._check_input_consistency(x, pos, edge_index)
print(len(pos))
if edge_index is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't know if this is nice to be kept here.. why radius and not knn? Do we really need a temporal graph? One could do a normal RadiusGraph and then add time as additional parameter

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. As for now I remove TemporalGraph

@@ -20,3 +21,4 @@
from .avno import AveragingNeuralOperator
from .lno import LowRankNeuralOperator
from .spline import Spline
from .gno import GNO
Copy link
Collaborator

Choose a reason for hiding this comment

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

GraphNeuralOperator full name

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

internal_func=internal_func,
external_func=external_func)
self.n_layers = n_layers
self.forward = self.forward_shared
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like a lot this forward separation, is there a way to combine the two?

Copy link
Member Author

Choose a reason for hiding this comment

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

In order to have an efficient way to store parameters (avoid to use torch.nn.ModuleList with the same model repeated n_layer times), another possible solution is using an if in the forward. Otherwise I can define another 2 classes: one for the shared_weights and one for the non shared_weights. Let me know what how to proceed

@@ -15,6 +15,7 @@
"AVNOBlock",
"LowRankBlock",
"RBFBlock",
"GraphIntegralLayer"
Copy link
Collaborator

Choose a reason for hiding this comment

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

GNOBlock

@dario-coscia
Copy link
Collaborator

@FilippoOlivo @gc031298 overall very nice, this looks a very nice integration in PINA! Just minor changes, I think we will be able to merge it soon :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-to-review Label for PR that are ready to been reviewed v0.2 implementation in v0.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graph class implementation
4 participants