-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[DOC] ExponentialSmoothing default method change from L-BFGS-B to SLSQP #6186
[DOC] ExponentialSmoothing default method change from L-BFGS-B to SLSQP #6186
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Quick question, may I clarify which of the following is the case:
None
translates toSLSQP
instatsmodels
alreadyNone
will translate toSLSQP
instatsmodels
at some future point, when a planned PR is going to be merged and the default will change- this, and
statsmodels
is not raising a deprecation warning - this, and
statsmodels
is, or will be raising a depercation warning
- this, and
Depending on what is the case here, we may have to take additional steps, in accordance with change/deprecation policy:
https://www.sktime.net/en/latest/developer_guide/deprecation.html
As a first step, I feel we should change the default from None
to what it currently is in statsmodels
. FYI @benHeid, @yarnabrina
@fkiraly based on the shared link above, it seems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
In that case, we should consider changing the sktime
default to "SLSQP"
(separate from this PR), to avoid behaviour switching again.
Should I go ahead and open a new PR switching the default value from None to SLSQP? @fkiraly |
Not sure whether we should. Since it is a very small change, how about this - you open a PR, and then we discuss with other core devs what they think? |
Reference Issues/PRs
While exploring #6175, realized that the default parameter for
method
in Exponential Smoothing should beSLSQP
. This also affects statsmodels documentation and there is already an open PR in their repo to fix this.What does this implement/fix? Explain your changes.
Changes
method='L-BFGS-B'
tomethod='SLSQP'
.Does your contribution introduce a new dependency? If yes, which one?
No
What should a reviewer concentrate their feedback on?
See statsmodels default
Did you add any tests for the change?
N/A
Any other comments?
N/A
PR checklist
For all contributions
How to: add yourself to the all-contributors file in the
sktime
root directory (not theCONTRIBUTORS.md
). Common badges:code
- fixing a bug, or adding code logic.doc
- writing or improving documentation or docstrings.bug
- reporting or diagnosing a bug (get this pluscode
if you also fixed the bug in the PR).maintenance
- CI, test framework, release.See here for full badge reference
See here for further details on the algorithm maintainer role.