-
-
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
[ENH] EnbPI based forecaster #6449
Conversation
7e68b6f
to
e95dfe4
Compare
@fkiraly should be ready for review :) |
Before merging check the dependencies.. |
For the EnbPI wrapper, we using just a single method of the EnbPI class in the aws-fortuna package. Unfortunately this package has a lot of soft dependencies, including JAX. Furthermore, currently, the package is not easily installable due to dependency conflicts: awslabs/fortuna#194 Thus, I wonder if we could copy paste the method conformal_intervals method from the EnbPI class of the aws-fortuna package and adapt it slightly (adaption would get rid off the checks that aren't required anymore since it is internal and replacement of jax.numpy by numpy directly. If copying and modifiying we need to include the original license and the notice statement. Not sure how we should do this: https://github.com/awslabs/fortuna?tab=Apache-2.0-1-ov-file#readme and https://github.com/awslabs/fortuna/blob/main/NOTICE @fkiraly any ideas? |
If it is a very small part only, it could go in a private submodule with a license file. We could also give the user the option to use |
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.
Looks good, some minor things:
- for
sktime
estimators, theclone
method (of the estimator) should be used, or configs will not be cloned. - the docstring should describe better how the algorithm is applied
- I would move it to an enbpi module.
compose
is for pipelines or similar, it does have some "ensembles" in it but I think in some cases these are misnomers, in others I would not have put it there.
Non-blocking: some typos
@fkiraly I addressed your changes and additionally also added a adapted version of the EnbPI implementation from aws-fortuna into the libs folder, together with the corresponding license and the NOTICE file that includes the copyright of aws-fortuna. I would appreciate a check if this is added correctly or if I should change the structure.. |
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.
Regarding the library:
- I would make it private, since it is not intended for public consumption
- call it
_aws_fortuna_enbpi
or similar as it is not the entire library, just a method from it README
should be markdown,md
- I would also suggest to add to the main
README
inlibs
. I would suggest to add a new section "snippets" or similar and explain that these are private- while you are at it, you could also fix the "distibution" type :-)
Also worth considering:
- instead, new folder
_libs_private
,libs_private
, or similar? - moving it into a folder close to the estimator, as it is the single dependency. Coupling-wise, might be better to use it directly from there?
What are your thoughts on handling private libraries?
Perhaps we should open an issue to discuss more widely with core devs, public and private libraries?
I addressed your comments and created a new issue #6648 |
@@ -5,6 +5,7 @@ | |||
__all__ = [ | |||
"HierarchyEnsembleForecaster", | |||
"ColumnEnsembleForecaster", | |||
"EnbPIForecaster", |
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.
left over?
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.
Yes..
@fkiraly Please wait before merging, I need to check if I introduced a Bug. I will ping you if everything is fine. |
@fkiraly bug should be fixed now. |
Reference Issues/PRs
Towards #6446, uses the aws-fortuna based approach together with tsbootstrap bootstraps
What does this implement/fix? Explain your changes.
Interfaces the aws-fortuna EnbPI implementation
Does your contribution introduce a new dependency? If yes, which one?
Yes aws-fortuna as well as third party library to use them (Jax==0.28.1)
What should a reviewer concentrate their feedback on?
Did you add any tests for the change?
get_test_params
Any other comments?
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
maintainers
tag - do this if you want to become the owner or maintainer of an estimator you added.See here for further details on the algorithm maintainer role.
For new estimators
docs/source/api_reference/taskname.rst
, follow the pattern.Examples
section.python_dependencies
tag and ensureddependency isolation, see the estimator dependencies guide.