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] Shapelet Transform improvements #185

Merged
merged 32 commits into from Mar 17, 2023
Merged

[ENH] Shapelet Transform improvements #185

merged 32 commits into from Mar 17, 2023

Conversation

TonyBagnall
Copy link
Contributor

@TonyBagnall TonyBagnall commented Mar 10, 2023

the main thing here is that the conversion from numpy to dataframe is removed. All that was happening was the numpy was getting put into a dataframe in the transform, then converted back to numpy in classifier. The base transformer does this conversion automatically, unless you set this argument in the constructor

super(RandomShapeletTransform, self).init(_output_convert=False)

other changes:

  1. Docstrings tidied up, dimensions changed to channels, all reference to DataFrames removed
  2. Helper classes for full ShapeletTransform made private
  3. Remove pointless tags

to do in a later PR
Order of classes in file changed to put RandomShapeleTransform first

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@TonyBagnall TonyBagnall added the transformations Transformations package label Mar 10, 2023
Copy link
Contributor

@GuzalBulatova GuzalBulatova left a comment

Choose a reason for hiding this comment

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

I've got a couple of comments and a general suggestion to break down the big PRs into smaller ones where possible.
Overall this is absolutely fantastic work! Definitely a significant improvement. The code flow is easy to read and I really appreciate the comprehensive documentation.

sktime/transformations/panel/shapelet_transform.py Outdated Show resolved Hide resolved
sktime/transformations/panel/shapelet_transform.py Outdated Show resolved Hide resolved
sktime/transformations/panel/shapelet_transform.py Outdated Show resolved Hide resolved
sktime/transformations/panel/shapelet_transform.py Outdated Show resolved Hide resolved
@MatthewMiddlehurst
Copy link
Member

I agree that it is extremely difficult to tell what has actually changed without going through the full file (both pre and post change). Maybe moving them around should be a separate thing.

@TonyBagnall
Copy link
Contributor Author

thanks for the feedback @GuzalBulatova, really helpful. I did wonder about the complexity of reviewing when moving classes, just wanted to reprioritise since the ShapeletTransformClassifier is there for legacy only, but it is purely cosmetic, I will shift them back. I have a more systemic issues about the tests for which I will raise an issue

@TonyBagnall
Copy link
Contributor Author

I agree that it is extremely difficult to tell what has actually changed without going through the full file (both pre and post change). Maybe moving them around should be a separate thing.

agreed, I will shift order on its own PR, hopefully more readable now

@TonyBagnall
Copy link
Contributor Author

TonyBagnall commented Mar 11, 2023

Im working on a notebook revamp for classification on a different branch, but I think as I am here with STC I will write a shapelet notebook, which make take a while

@TonyBagnall
Copy link
Contributor Author

I have added a notebook on using the transform. I want to put more details in, but I need to work out how it interacts with the classification notebooks I'm changing on #123 so would like to leave it at this for now.

Copy link
Member

@MatthewMiddlehurst MatthewMiddlehurst left a comment

Choose a reason for hiding this comment

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

I'm fine with the changes, but the shapelet notebook may conflict with the previous notebook PR. Just not getting picked up because it's in a different dir.

@TonyBagnall TonyBagnall dismissed GuzalBulatova’s stale review March 17, 2023 10:14

made the changes, hope its ok to dismiss

@TonyBagnall TonyBagnall merged commit f01344d into main Mar 17, 2023
13 checks passed
@TonyBagnall TonyBagnall deleted the ajb/stc branch March 17, 2023 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
transformations Transformations package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants