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

Fix PyG converter crash when no property is extracted at all #1225

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

mewim
Copy link
Member

@mewim mewim commented Jan 31, 2023

No description provided.

@mewim mewim requested a review from ray6080 January 31, 2023 17:03
@mewim mewim force-pushed the pyg-minor-fix branch 2 times, most recently from 7cb4435 to 120cd90 Compare January 31, 2023 17:43
@@ -182,6 +183,8 @@ def __mark_property_unconverted(self, label, prop_name):
def __populate_edges_dict(self):
# Post-process edges, map internal ids to positions
for r in self.rels:
if (r[0], r[1]) not in self.internal_id_to_pos_dict or (r[2], r[3]) not in self.internal_id_to_pos_dict:
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems there should be a warning here because it only happens when user fail to return the src/dst node for a rel which is an undefined behaviour (when converting to pyg) in my opinion. Up to u to decide.

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 just realized that now with the behavior of returning unconverted properties, it makes more sense to always return the full graph structure regardless of whether the properties can be converted. If no property is converted, the position will correspond to the index of unconverted properties lists. So I have removed this check and always maintain the mappings regardless if whether a property can be extracted. I also added a test case for this.

@mewim mewim merged commit 497235d into master Jan 31, 2023
@mewim mewim deleted the pyg-minor-fix branch January 31, 2023 19:56
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.

None yet

2 participants