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 relation prediction with inverses #1304

Merged
merged 3 commits into from
Aug 9, 2023

Conversation

mberr
Copy link
Member

@mberr mberr commented Aug 8, 2023

Fix #1303

@mberr mberr changed the title Fix relation prediction with inverses 🔥🪞 Fix relation prediction with inverses Aug 8, 2023
@@ -61,6 +65,10 @@ def invert_(self, batch: torch.LongTensor, index: int = 1) -> torch.LongTensor:
batch[:, index] += 1
return batch

# docstr-coverage: inherited
def is_inverse(self, ids: torch.LongTensor) -> torch.BoolTensor: # noqa: D102
Copy link
Member

Choose a reason for hiding this comment

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

somewhere there is also the inverse index generator function, should these be coupled?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is exactly what this class is for 😉

The full inversion combines _map and invert_ defined in this concrete class, DefaultRelationInverter , in map in the base class, RelationInverter.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is mostly a fix for the prediction dataframe; there are still some inconsistencies to fix (e.g. that inverse relations are stored in a dataset instead of solely encapsulated in a model; that tf.relation_to_id contains only the relation ids before remapping, etc.), cf. this long-standing PR: #752

@mberr mberr requested a review from cthoyt August 9, 2023 17:38
@mberr mberr merged commit eb752ad into master Aug 9, 2023
@mberr mberr deleted the fix-relation-prediction-with-inverses branch August 9, 2023 19:39
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.

ValueError: All arrays must be of the same length, While predicting a relation with CompGCN
2 participants