-
-
Notifications
You must be signed in to change notification settings - Fork 179
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
🔮〰️ Update Prediction Filtering #1048
Conversation
…en/pykeen into update-prediction-filtering
|
||
def filter(self, df: pd.DataFrame) -> pd.DataFrame: | ||
""" | ||
Filter out known triples. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should mention in docstring this is not an in-place operation, maybe should give the possibility for both ways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, we do not have a use-case for in-place operations, so I would defer the implementation until then 😉
), | ||
test_elements=mapped_triples, | ||
) | ||
.cpu() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe better to return the torch tensor then just cpu().numpy() in the place where it gets used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, we need cpu().numpy()
everytime we call this function, so I would leave it as it is for now.
Besides minor code comments I think this is fine, if it matches existing functionality then great. Will you later add some more high-level documentation when you're ready to expose to the public? |
Pull request was converted to draft
This (draft) PR introduces support for more complex prediction filtering, e.g., with multiple filter triples sets.
It does not (yet) expose this functionality to public.
related to #1040