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

[ENH] adding back MrSEQL to sktime #5178

Merged
merged 3 commits into from Sep 17, 2023
Merged

[ENH] adding back MrSEQL to sktime #5178

merged 3 commits into from Sep 17, 2023

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Aug 29, 2023

Fixes #4296, adds back MrSEQL to sktime.

See discussion there regarding temporary removal and complications in soft dependency isolation which now should all be resolved.

@fkiraly fkiraly added interfacing algorithms Interfacing existing algorithms/estimators from third party packages module:classification classification module: time series classification labels Aug 29, 2023
@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 29, 2023

Looks like it works out of the box - FYI @lnthach.

Noting, the mrseql package does not seem to have its dependencies set properly? It depends on numba, and will break without numba, but pip install mrseql does not install numba.

Not a problem, since sktime estimators manage their own dependencies and it's added there, but it would be an issue for direct users of the mrseql package.

@lnthach
Copy link
Contributor

lnthach commented Aug 30, 2023

Thanks @fkiraly . I will look into the dependency issue. Though I didn't use numba when implementing MrSEQL. Probably because MrSEQL depends on sktime for its SFA implementation.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 30, 2023

Probably because MrSEQL depends on sktime for its SFA implementation.

Yes, MrSEQL depends on sktime's SFA which in turn depends on numba (sktime itself does not depend on numba, it is a soft dependency of SFA)

@fkiraly
Copy link
Collaborator Author

fkiraly commented Aug 30, 2023

hm - hold a moment, it does depend on numba only if symrep contains the "sfa" string!

This is an interesting case, as this makes the default configuration not numba or sktime dependent!

I think it is worth leaving the dependencies then, as thas allows users to use part of SFA without numba, which comes with a significant installation and maintenance overhead, and instead raising an error message if SFA is used (which already happens afaik).

@fkiraly fkiraly merged commit ab9746e into main Sep 17, 2023
24 checks passed
@fkiraly fkiraly deleted the mrseql branch September 17, 2023 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interfacing algorithms Interfacing existing algorithms/estimators from third party packages module:classification classification module: time series classification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] MrSEQL classifier revisited - with dependency isolation
2 participants