-
-
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] Benchmarking interface v2 based on kotsu
package
#2977
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
f0004cd
to
5b3f380
Compare
} | ||
], | ||
"source": [ | ||
"results_df = benchmark.run(\"./forecasting_results.csv\")\n", |
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.
In my run, forecasting_results.csv
shows up in the root directory of the repo. In this by design? If yes, I think it's better in the working directory.
Though I can easily pd.read_csv
this file, is there a reason to write to a output file?
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.
In my run,
forecasting_results.csv
shows up in the root directory of the repo. In this by design? If yes, I think it's better in the working directory.
I don't think there was much thought gone into where it should be saving yet - the example notebook is intended to be an example, and the hope would be users would configure where they wanted to save their results.
Though I can easily
pd.read_csv
this file, is there a reason to write to a output file?
When writing kotsu
we generally wanted results to be persisted; you run some experiments one day, and we want those results to still be there when we come back to the project/work say a week later. Leaving results in memory isn't a reliable way of achieving that.
(Slides from dev-days-2022 talk on 12-7-22 on "sktime - easy benchmarking", which is relevant to this PR) |
@DBCerigo - FYI sktime template pattern motivation / explanation https://github.com/sktime/sktime-workshop-pydata-london-2022/blob/main/notebooks/2_basic_extensions.ipynb |
sktime/benchmarking/benchmarks.py
Outdated
""" | ||
from typing import Callable, Optional, Type, Union | ||
|
||
import kotsu |
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.
I would suggest to move the import inside the constructor and inside the run
.
To complete soft dependency encapsulation, kindly add a _check_soft_dependencies
at the start of the __init__
.
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.
Done in 4e18ac4
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 great as a minimal example!
I have suggestions above, the blocking one is the one on isolating the soft dependency.
The other one would be nicer from a pattern/design perspective imo.
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.
I also think it is a problem that estimators and their parameters are separated.
I'd rather have objects passed than classes and their parameters separately.
Unfitted objects carry the same information - they are the class plus their kwargs (can be retrieved by get_params
)
See discussion in #2804
5b3f380
to
4e18ac4
Compare
Now ready for re-review @fkiraly. Some replies made directly on review comments above, and another #2804 (comment) in #2804. PR checklist
|
b09507d
to
e094aec
Compare
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.
I suppose this is good, as a design study!
Blocking change request:
- please add docstrings to public methods and user facing classes
Questions:
- how would carrying out tests integrate with this? You need access to individual predictions for that.
- how would storing partial results on the HD or via a data interface class work?
- I do not fully understand what type or signature you require for
dataset_loader
, could you elaborate?
Just to confirm, you'd like the doc strings extended to contain
What do you mean by "tests"?
Can you clarify what you mean by "partial results"?
This implementation isn't intending to add a new object or interface for a |
Yes,
Apologies, I meant "statistical hypothesis tests", on comparing performance of estimators. Not a blocking comment, anyway.
For instance, predictions, of m out of n estimators involved in the experiment - similar to the old framework.
Ah, I see - docstrings would be helpful that detail the assumptions, then. |
e094aec
to
3e28d29
Compare
🎉 welcome back, @DBCerigo 🎉 |
cb891ea
to
3b6d803
Compare
Great! Good to have you back. There's one point to address, it is dependency isolation. If
in the code base. Also see |
@fkiraly thanks for the pointers for the test skips - all implemented now. Let's wait until we have 2+ subclasses of the base class before doing any more design changes, as having multiple examples will make it plainer to see and verify the usefulness of any further design changes. So I would suggest merging this PR as is. Then implementing and merging 1+ more benchmarks (I think @TNTran92 has a PR for classifiers already? though likely needs some retweaking now), then reviewing the implementation as a whole. |
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.
Massive PR, extremely useful functionality!
Comments are either addressed, or make sense to address separately, e.g., minor rewors to the interface.
kotsu
package
Reference Issues/PRs
See previous discussions #2884 and #2805 with previous examples of integrating kotsu into sktime by @TNTran92 and @DBCerigo
The current PR is intended to provide an example of how benchmarking could be implemented by wrapping kotsu in such a way that users only need to deal with sktime objects.
What does this implement/fix? Explain your changes.
This PR provides a candidate benchmarking interface for sktime estimators/datasets by wrapping kotsu.
Currently only supports forecasters, but is intended to be flexible.
It is intended to allow users to create their own benchmarks by adding sets of estimators and tasks.
Does your contribution introduce a new dependency? If yes, which one?
kotsu
What should a reviewer concentrate their feedback on?
Any other comments?
PR checklist
For all contributions
For new estimators