-
Notifications
You must be signed in to change notification settings - Fork 50
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 Campaign.posterior_stats
#504
Open
Scienfitz
wants to merge
17
commits into
main
Choose a base branch
from
feature/posterior_statistics
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
2183bc7
Add posterior_statistics
Scienfitz f87d5ad
Add test
Scienfitz 43dc9a4
Update user guide
Scienfitz e9a2c45
Update CHANGELOG.md
Scienfitz f7e5eb1
Rework statistics selection
Scienfitz f6519b3
Rename method
Scienfitz 43d3142
Silence MyPy
Scienfitz fbd4abc
Import torch locally
Scienfitz eefb091
Use sequences in test
Scienfitz 9662c27
Rework typing
Scienfitz 66184a1
Apply suggestions from code review
Scienfitz 736b939
Make candidates optional
Scienfitz 085213a
Change `variance` to `var`
Scienfitz b7c9b13
Move disabling of gradients
Scienfitz 09dbfb2
Enable quantiles for PosteriorList
Scienfitz 61703ae
Fix docs
Scienfitz e78fb53
Move core logic to surrogates
Scienfitz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
One more general comment: Just like we have
Campaign.get_posterior
fetch the object from the underlyingRecommender
, wouldn't it actually make sense to implement the core logic ofposterior_stats
in theBayesianRecommender
class and then makeCampaign
just reuse it? That way, it would also become available at the recommender level, analogous toBayesianRecommender.posterior
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.
@AdrianSosic
Can you point me to where is
BayesianRecommender.posterior
or did you get this confused? Or is it because of the Adapter? As far as I can see its only part of surrogatesThere 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.
Sorry, I mixed up a few things.
Campaign.get_posterior
fetches the surrogate via the recommender and then evaluates it. So similarly,Campaign.posterior_stats
could again fetch the surrogate from the recommender and then just call theposterior_stats
there (which would contain the core logic you've implemented)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.
ok I pushed a proposal commit for that, seems to work
gets rid of a workaround for
PosteriorList
because the composite just calls the single surrogates, which however are initialized with single-objectives and a posterior list doesnt appear anywhere