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

🔮☕ Custom Prediction Filtering #1090

Merged
merged 67 commits into from
Aug 29, 2022
Merged

🔮☕ Custom Prediction Filtering #1090

merged 67 commits into from
Aug 29, 2022

Conversation

mberr
Copy link
Member

@mberr mberr commented Aug 25, 2022

This PR refactors the prediction filtering (again) to better support custom filtering.

Dependencies

@mberr mberr marked this pull request as ready for review August 26, 2022 12:24
@mberr mberr changed the title Custom Prediction Filtering 🔮☕ Custom Prediction Filtering Aug 26, 2022
@mberr mberr requested a review from cthoyt August 26, 2022 13:11
@mberr mberr marked this pull request as draft August 26, 2022 13:41
@mberr mberr marked this pull request as ready for review August 26, 2022 13:45
@@ -203,7 +203,7 @@ def predict_hrt_uncertain(
Example Usage::

from pykeen.pipeline import pipeline
from pykeen.models.predict import predict_hrt_uncertain
from pykeen.models.uncertainty import predict_hrt_uncertain
Copy link
Member

Choose a reason for hiding this comment

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

does this module now rely on pykeen.predict? sort of complicated, maybe this module should also be in a predict subpackage?

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 am unsure about this 😅

The uncertainty module only wraps the model's predict_{h,r,t}, but does not offer the flexibility of the general predict module.

Copy link
Member

Choose a reason for hiding this comment

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

okay then leave it for now

@cthoyt
Copy link
Member

cthoyt commented Aug 26, 2022

I'm still hesitant about throwing away the nice high-level functions but I don't want to get in the way since you're on a roll

@mberr mberr requested a review from cthoyt August 29, 2022 09:15
@cthoyt cthoyt merged commit 6822639 into master Aug 29, 2022
@cthoyt cthoyt deleted the refactor-predict branch August 29, 2022 12:29
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