-
Notifications
You must be signed in to change notification settings - Fork 207
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] Added more prominent examples of using aeon with sklearn #2705
base: main
Are you sure you want to change the base?
[DOC] Added more prominent examples of using aeon with sklearn #2705
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Thank you for contributing to
|
@@ -29,7 +29,7 @@ | |||
}, |
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.
Line #12. "Graph of neighbors for the first pattern in testing set with EDR distance on 2D"
Just caught that we are missing a space after "2D" for the print to be correctly formatted here, would be nice to fix this if you could.
Reply via ReviewNB
@@ -29,7 +29,7 @@ | |||
}, |
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 don't think this section convey the right message. This kinda implies that it would make sense to use time series data with sklearn estimators by just flattening it, which can hardly be recommended for time series data in general.
I would either remove it or add an example on how to convert 3D to 2D using an aeon transformer estimator (e.g. shapelet, rocket, catch22 etc...)
Reply via ReviewNB
@@ -29,7 +29,7 @@ | |||
}, |
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.
Line #3. knn_regressor.fit(X_train_2D_flat, y_train_3D)
If the classification case uses a distance-based approach, I would rather have regression to use an aeon transformation to make a X_train_2D to use with the regression. Then you could show how to combine the two estimators inside a sklearn pipeline.
Reply via ReviewNB
@@ -29,7 +29,7 @@ | |||
}, |
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.
Line #10. X_test_2D = X_test_3D.reshape(X_test_3D.shape[0], -1)
These 2D arrays are not used in this cell, should be removed
Reply via ReviewNB
@@ -29,7 +29,7 @@ | |||
}, |
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.
To add some example diversity, I would rather have an example of how to define a pipeline using an aeon transformer estimator followed by a sklearn estimator, which can then be used into the cross validation or to fit/predict
Reply via ReviewNB
PR Description for Issue #1921
Pull Request Title:
[DOC] Added more prominent examples of using aeon with sklearn
Reference Issues/PRs
Fixes #1921
What does this implement/fix?
This PR enhances the
sklearn_distances.ipynb
notebook by:Does your contribution introduce a new dependency?
No new dependencies were introduced.
Any other comments?
This aims to make aeon’s usage in sklearn workflows more accessible and well-documented. Let me know if any refinements are needed! 🚀
Here's the corrected checklist for your PR:
PR Checklist
For all contributions
For documentation updates