Changing Default Values of plotting and prediction percentiles

[edwinnglabs]

Description

Having prediction percentiles=None is quite annoying since I find more often the reason to have LGT/DTLFull is to get reliable inference. Each time if I want to create a new DLTFULL(after testing DLTMAP), i need to figure out the right arg.

Fixes # (issue)

Change prediction_percentiles=None = prediction_percentiles=[5,95] and some default plotting value changed to make it less input required if we always use default prediction outcomes.

Please delete options that are not relevant.

• Change related tutorial/docs update for cosmetic purpose (should not trigger any error)
• restore prediction outcomes label by using input prediction percentiles directly
• set prediction percentiles default as [5, 95] internally

How Has This Been Tested?

Since it is plotting, no test is related for this.

[edwinnglabs]

change of notebook will be done after approval. it is more for cosmetic than any syntax/runtime error.

[wangzhishi]

this also requires a docs update? the update is done or not yet?

[steveyang90]

I'd very strong urge against this. Default args being mutable is generally a bad idea. (I think this is a good description of why: https://docs.python-guide.org/writing/gotchas/)

I'd suggest setting following the design of the otther args and set the default in _set_default_base_args()

[steveyang90]

@edwinnglabs @wangzhishi Also worth noting that changing the default value here changes the output for LGTFull and isn't consistent with the other classes. That's ok, but we should make sure that that's what we really want. I feel the end user setting the interval arg is an ok trade off for consistent api behavior. Also because [5, 95] is sort of arbitrary lower and upper bounds?

[edwinnglabs]

this also requires a docs update? the update is done or not yet?

As mentioned, they are not material since i am adding an argument on top of a None default. So the old notebooks should work perfectly. it's just cosmetic since we are putting an input that is already default (in this refined version). But I can do that ONLY after we agree this is the change. @wangzhishi

[edwinnglabs]

@edwinnglabs @wangzhishi Also worth noting that changing the default value here changes the output for LGTFull and isn't consistent with the other classes. That's ok, but we should make sure that that's what we really want. I feel the end user setting the interval arg is an ok trade off for consistent api behavior. Also because [5, 95] is sort of arbitrary lower and upper bounds?

are you just against the idea of renaming the output to x_upper and x_lower or just against the idea having a list in default argument? @steveyang90

[steveyang90]

Hey sorry let me clarify a bit, because I think there are multiple points!

We should definitely not set the default explicitly in the method signature to prediction_percentiles=[5,95]. However, if we feel strongly about [5,95] being the default, we should keep the default as None, but instead set the behavior of prediction_percentiles=None to mean [5,95] by setting the default values within _set_default_base_args(). For example, this is the same way we set a private var called self._regressor_sign when the arg for regression_sign=None.

The second part is the unit test that you updated. Where is "prediction_lower" and "prediction_upper" coming from? Was this a recent change? Last I recall, we were using the actual ints for the intervals since users can put in an arbitrary list of ints, not just upper and lower?

[edwinnglabs]

Hey sorry let me clarify a bit, because I think there are multiple points!

We should definitely not set the default explicitly in the method signature to prediction_percentiles=[5,95]. However, if we feel strongly about [5,95] being the default, we should keep the default as None, but instead set the behavior of prediction_percentiles=None to mean [5,95] by setting the default values within _set_default_base_args(). For example, this is the same way we set a private var called self._regressor_sign when the arg for regression_sign=None.

The second part is the unit test that you updated. Where is "prediction_lower" and "prediction_upper" coming from? Was this a recent change? Last I recall, we were using the actual ints for the intervals since users can put in an arbitrary list of ints, not just upper and lower?

So for the first point, I would prefer to set the percentiles default as [5, 95] using the _set_default_base_args method you mention. The advantage of using MCMC is to generate inference. I think it makes sense to make it as default. It also helps a cleaner demo where we don't need to put [5, 95] as an argument everywhere

As regard to the second point, we changed to upper and lower some version ago. I find it is annoying to keep tracking what we input originally in the model to pipe the same into plotting and we will only plot one band with two end points anyway. So I feel converting them into ['upper' and 'lower'] may actually make more sense. However, I don't completely oppose to use [5, 95]. I just feel it may take some effort to make sure user has a consistent set of upper and lower bound. For example do we allow user put prediction_perfcentile=[10,20,30]? And if so, it needs some manual input for the plotting. For the upper and lower approach, we can always be lazy in plotting since the labels are always upper and lower.

[steveyang90]

Cool, I think we're agreed on the first point then!

Regarding the second point, I see what you mean with plotting. So maybe the solution is to be more restrictive with prediction_percentiles to always just have two elements which corresponds to upper and lower bounds, rather than allowing an arbitrary list?