[timeseries] Avoid masking the 'scaler' param with the default 'target_scaler' value #5131
Conversation
…aler' in MLForecast models
shchur
added
bug
timeseries/src/autogluon/timeseries/models/autogluon_tabular/mlforecast.py
Outdated
Show resolved
Hide resolved
|
|
||
| # Support "scaler" for backward compatibility | ||
| scaler_type = model_params.get("target_scaler", model_params.get("scaler")) | ||
| if "scaler" in model_params: |
There was a problem hiding this comment.
What happens if user provides both scaler and target_scaler? Should we raise or warn?
There was a problem hiding this comment.
Also, does this imply any documentation changes?
There was a problem hiding this comment.
Before this PR: scaler is always ignored, only target_scaler matters.
This PR (at the time of review): scaler takes precedence over target_scaler (probably not ideal)
I have updated the PR so that target_scaler takes precedence over scaler - that seems more reasonable to me.
We don't need to update the docs - they already only reference the new name target_scaler.
There was a problem hiding this comment.
I've also added a warning to avoid introducing a potentially breaking change in a hotfix release.
There was a problem hiding this comment.
@abdulfatir, regarding the warnings category, DeprecationWarnings are often silenced by the default warning filters (see #3463 or PEP565), so I would prefer to keep the UserWarning here to ensure that it's displayed.
abdulfatir
left a comment
abdulfatir
left a comment
There was a problem hiding this comment.
Looks good to me, but at this stage maybe it's better to just make it a deprecation warning for future clarity. I'll let you decide.
2 participants
Issue #, if available:
Description of changes:
scalerhyperparameter to choose the scaling option in MLForecast models. In v1.3.0, we renamed this hyperparameter totarget_scalerfor consistency with other models. We intended to support thescalerhyperparameter as well for backwards compatibility. However, because of a bug in the logic,scalerwas always shadowed by the default value of thetarget_scaler, so the old parameter name got effectively deprecated. This can result in unexpected performance changes to the users. This PR fixes this problem.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.