Skip to content

Conversation

@edwinnglabs
Copy link
Collaborator

@edwinnglabs edwinnglabs commented Oct 25, 2020

Description

Changing default value of prediction percentile.

Fixes # (issue)

Having a default percentiles make more sense as a Full Bayesian Estimator. It also changes default plotting arg to reduce code in many demos.

  • This change requires a documentation update

How Has This Been Tested?

I don't think additional unit test needed unless we want to add basic test for plotting utils

@edwinnglabs edwinnglabs added documentation Improvements or additions to documentation review needed need someone to review labels Oct 25, 2020
@edwinnglabs
Copy link
Collaborator Author

@steveyang90 as requested, I re-submit the original one that just changing the default where this time I use internal module to avoid mutable inputs.
@wangzhishi could you help refreshing all the docs, indeed this is very minor where all you need is to delete all the default inputs like pred_col='prediction' and prediction_percentiles=[5,95] in all the Full Estimator

Copy link
Collaborator

@steveyang90 steveyang90 left a comment

Choose a reason for hiding this comment

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

LGTM!

@edwinnglabs edwinnglabs merged commit 589eefc into master Oct 26, 2020
@edwinnglabs edwinnglabs deleted the change-percentiles-default branch October 28, 2020 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation review needed need someone to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants