Skip to content
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

[BUG] fix BoxCoxTransformer failure after scipy 1.11.0 #4770

Merged
merged 15 commits into from Jul 2, 2023
Merged

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Jun 25, 2023

Fixes #4769 by using scipy's on-board box-cox-fitter for methods "mle" and "pearsonr" and enforcing positive values.

The reason for the failure was use of negative values in some interface test cases, which previously would produce garbage results but scipy would not complain. As of 1.11.0, it complains (albeit with a confusing error message that makes it hard to track down that it is in fact from negative values in some test cases). See further discussion here: scipy/scipy#18761

This is now solved by introducing an enforce_positive argument to BoxCoxTransformer, which ensures positive values before fitting. This can be turned off to obtain the previous behaviour, but for positive values it will be the same (although with an additional np.sign and np.abs calculation that should be fast).

Also makes the following changes, while we're cleaning this up:

  • The sktime box-cox-fitter in BoxCoxTransformer was not using scipy's since the latter did not have settable bounds when the former was originally implemented. I have hence replaced it with scipy's own box-cox-fitter and using a bounded optimizer if bounds are passed, from scipy 1.7.0 on where it exists in the form we need. Pre-1.7.0, I've left the old behaviour.
  • improved docstring - general improvements, removed some wrong math statements, made clear what the choice of method means and when it interfaces scipy
  • added a "fixed" method, which allows to use the transformer with a fixed parameter - or tune it via grid search.
  • input checks on method

Note: the fix is behaviour changing, but imo does not require deprecation because it changes some behaviour from buggy/misleading (only in the sub-case where negative values were passed) to sensible.

@fkiraly fkiraly added module:transformations transformations module: time series transformation, feature extraction, pre-/post-processing bugfix Fixes a known bug or removes unintended behavior labels Jun 25, 2023
@fkiraly
Copy link
Collaborator Author

fkiraly commented Jun 26, 2023

update: just learnt that an initial bracket need not contain bounds - so I simply set the bracket to bounds if provided, while passing bounds if provided also to the optimizer (constrained)

@fkiraly fkiraly merged commit b1bc66a into main Jul 2, 2023
24 checks passed
@fkiraly fkiraly deleted the boxcoxfix branch July 2, 2023 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a known bug or removes unintended behavior module:transformations transformations module: time series transformation, feature extraction, pre-/post-processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] BoxCoxTransformer fails with scipy.optimize._optimize.BracketError since scipy 1.11.0
1 participant