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] iisignature backend option for SignatureTransformer #5398

Merged
merged 23 commits into from Nov 4, 2023

Conversation

sz85512678
Copy link
Contributor

@sz85512678 sz85512678 commented Oct 11, 2023

Reference Issues/PRs

Fixes #1591.

This PR also adds an option to choose no augmentation, currently the code returns an error if augmentations is None -- the user will be forced to choose an augmentation.

What does this implement/fix? Explain your changes.

Fixes a bug where a runtime error is returned if each point of the time-series has dimension more than 25.
An enhanced flexibility to allow to user to choose no stream augmentation.

Does your contribution introduce a new dependency? If yes, which one?

Soft dependency of the package iisignature is needed for the fix of #1591.

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

No

Any other comments?

PR checklist

For all contributions
  • [x ] I've added myself to the list of contributors with any new badges I've earned :-)
    How to: add yourself to the all-contributors file in the sktime root directory (not the CONTRIBUTORS.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 plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
    See here for full badge reference
  • Optionally, I've added myself and possibly others to the CODEOWNERS file - 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.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
For new estimators
  • I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
  • I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.
  • If the estimator relies on a soft dependency, I've set the python_dependencies tag and ensured
    dependency isolation, see the estimator dependencies guide.

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

I would request a few things around soft dependency handling, and "no breaking changes" - the philosophy is, no existing code for any user should change unexpectedly. So, I would suggest:

  • to the signature transformer and relevant utility functions, add a backend parameter or similar, the default is as current and one option is "iisignature".
  • the backend is set to iisignature only if the option is set
  • the iisignature soft dependency should be added to the same dependency sets in pyproject.toml that also contains esig.
  • there should be a soft dependency check in the init of the signature transformer. iisignatures should be checked only if the backend parameter is set to it, with an informative error message (that tells the user that the backend requires it), for this we could use _check_soft_dependencies.

@fkiraly fkiraly changed the title Fixes #1591 [ENH] iisignature backend option for SignatureTransformer Oct 11, 2023
@fkiraly fkiraly added module:transformations transformations module: time series transformation, feature extraction, pre-/post-processing enhancement Adding new functionality labels Oct 11, 2023
…signature backend parameters and check for soft-dependency
@sz85512678
Copy link
Contributor Author

Thank you for the detailed suggestions, they do make sense. I have updated the PR with your comments!

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Only one blocking comment: The new dependency should have an upper bound.

The other comments are not blocking from my side.

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 14, 2023

hm, another remark: in get_test_params, we should test both backends.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@fkiraly
Copy link
Collaborator

fkiraly commented Nov 1, 2023

Here's how I'd proceed: let's see what the tests do. If they run through, then all is fine. If not, we can exceptionally merge without the dep for testing and add a note to the docstring.

@sz85512678
Copy link
Contributor Author

Here's how I'd proceed: let's see what the tests do. If they run through, then all is fine. If not, we can exceptionally merge without the dep for testing and add a note to the docstring.

Thank you. Are the tests already running? Sorry I am not sure how everything works -- (what am asking is if there is anything further I need to do now or simply wait).

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 1, 2023

Thank you. Are the tests already running?

Hm, they didn't seem to have run. I restarted them.

@sz85512678
Copy link
Contributor Author

It looks like the a few checks failed due to the numpy dependency of the iisignature as expected; There's some code quality issue from black, etc. Is the way to fix those to be simply running black etc. of the code before committing?

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 2, 2023

Is the way to fix those to be simply running black etc. of the code before committing?

Yes, although some errors you have to fix manually, e.g., too long lines of code or grammar issues with docstrings.

Here's the full guide on setting up code formatting checks locally:
https://www.sktime.net/en/stable/developer_guide/coding_standards.html

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the iisignature dependency from pyproject.toml, and added a warning message to the user about reliability. That should take care of the issues mentioned previously.

@fkiraly fkiraly merged commit 8035daa into sktime:main Nov 4, 2023
49 checks passed
@sz85512678
Copy link
Contributor Author

Thank you! Sorry for the late response, really appreciate your help for my first PR.

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 8, 2023

Well, thank you! We really appreciate your first PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding new functionality module:transformations transformations module: time series transformation, feature extraction, pre-/post-processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] RuntimeError when SignatureTransformer is run on large datasets
3 participants