Add Campaign.posterior_stats
#504
Conversation
Scienfitz
force-pushed
the
feature/posterior_statistics
branch
2 times, most recently
from
bf9d6d4 to
1302816
Compare
Scienfitz
changed the title
Campaign.posterior_statisticsCampaign.posterior_stats
There was a problem hiding this comment.
Hi @Scienfitz, here my comments regarding the code. Will now also have a quick look at the docs/tests
docs/userguide/campaigns.md
Outdated
| @@ -125,7 +125,47 @@ far. This is done by setting the following Boolean flags: | |||
| `pending_experiments` can be recommended (see [asynchronous | |||
| workflows](PENDING_EXPERIMENTS)). | |||
|
|
|||
| ### Caching of recommendations | |||
| ### Prediction Statistics | |||
There was a problem hiding this comment.
I'd say let's use Posterior Statistics. For once it's the correct term and consistent with the method name, and second I think that "prediction statistics" is technically really wrong, because a prediction cannot have a statistic (since it's already a realization)
| ### Prediction Statistics | |
| ### Posterior Statistics |
There was a problem hiding this comment.
main motivation in the heading here is not the technically correct term (the method is named technically correct)
but there is an argument to have about what unexperienced users are looking for, and there statistics about the predictions imo is something most people know or can phrase even without deeper technical knowledge, while the posterior reaches much less people
There was a problem hiding this comment.
another opinion on this renaming might enable us to make a quick decision @AVHopp
Prediction StatisticsTarget StatisticsPosterior StatisticsPredictive Statistics- anything better?
again, my problem with Posterior Statistics is that I don't think many unexperienced people will associate this with the feature they are looking for
There was a problem hiding this comment.
I'd be fine with Predictive Statistics
There was a problem hiding this comment.
One more general comment: Just like we have Campaign.get_posterior fetch the object from the underlying Recommender, wouldn't it actually make sense to implement the core logic of posterior_stats in the BayesianRecommender class and then make Campaign just reuse it? That way, it would also become available at the recommender level, analogous to BayesianRecommender.posterior
There was a problem hiding this comment.
@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 surrogates
There was a problem hiding this comment.
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 the posterior_stats there (which would contain the core logic you've implemented)
There was a problem hiding this comment.
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
| # Assert no NaN's present | ||
| assert not stats.isna().any().any() | ||
|
|
||
| # Assert correct error for unsupported statistics |
There was a problem hiding this comment.
Basically analogous to what you suggested in the cardinality PR. From here onwards, the tests are actually not really related to the actual input and only test for errors. Wouldn't it make sense to split it up and thereby separate valid and invalid cases into two isolated tests?
There was a problem hiding this comment.
i) the cardinality PR in the end did not separate the tests but amde one big test
ii) i would have to rewrite the entire long parameterization (main motivation to just append the invalid tests tot his one)
There was a problem hiding this comment.
ii) but if I'm not mistaken, the test don't need anything specific from that parametrization. So in that sense, the validations are also executed too often for no reason. If you separated the parts, the validation would not require a parameterization in the first place
There was a problem hiding this comment.
hmm thats debatable, arguably its better to test all kinds of surrogates and objectives also for the invalid tests?
Also: Given that these tests are a fraction of the cost thats needed for creating the campaign and the stat computation, why even bother?
Scienfitz
force-pushed
the
feature/posterior_statistics
branch
from
9ed9a3a to
cfbec87
Compare
Co-authored-by: AdrianSosic <[email protected]>
Scienfitz
force-pushed
the
feature/posterior_statistics
branch
from
c7e9fb6 to
09dbfb2
Compare
Scienfitz
force-pushed
the
feature/posterior_statistics
branch
from
e43d88c to
61703ae
Compare
posterior_statsto theCampaign, returning a data frame of statistics about the target predictionsCurrently, for desirability objectives it is hardcoded that the statistics are for a single target called "Desirability", consistent with what
transformdoes. Once posterior transforms are available, this logic needs to be adapted and it is expected that there is a flag inDesirabilityObjectivethat decides between scalarization or posterior_transforms. The latter case can then be treated exactly like all other targets.