Skip to content

[ENH] migrate InceptionTime classifier and example (from sktime-dl) #3003

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Apr 16, 2023
Merged

[ENH] migrate InceptionTime classifier and example (from sktime-dl) #3003

merged 11 commits into from
Apr 16, 2023

Conversation

tobias-weiss-ai-xr
Copy link
Contributor

@tobias-weiss-ai-xr tobias-weiss-ai-xr commented Jul 14, 2022 •
edited by fkiraly
Loading

Towards #3351

Migrates InceptionTime classifier, network and the corresponding jupyter notebook from sktime-dl, with minor changes to work within sktime.

Don't know if you have plans to port everything from sktime-dl one-by-one or in a single big effort.
In the latter case, please just discard my PR.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@fkiraly
Copy link
Collaborator

fkiraly commented Jul 14, 2022

Don't know if you have plans to port everything from sktime-dl one-by-one or in a single big effort.

I don't know about that for sure, I think we wanted to do it step by step? @TonyBagnall can tell you more.

In either case, any help is appreciated!

@TonyBagnall
Copy link
Contributor

its probably easier to do it one or two at a time, so please go ahead, thanks.

fkiraly
fkiraly requested changes Jul 15, 2022
View reviewed changes
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.

Excellent, thansk for the port!

May I suggest one thing, to avoid unstructured clutter in the examples dir:
could you rename the notebook to sth that makes clear it is about classification, sub-topic neural networks? E.g., 02a_classification_deeplearning_inceptiontime?

Or something similarly structured so that a lexicographic sort makes clear where it's from.
You may also like to think about order and naming in the context of the other notebook(s) you are adding - for instance, doesn't need to be 02 a as long as there is a consistent, logical order to things.

@tobias-weiss-ai-xr tobias-weiss-ai-xr force-pushed the InceptionTime branch 6 times, most recently from 7136ed9 to 25fed29 Compare July 15, 2022 15:18
@tobias-weiss-ai-xr tobias-weiss-ai-xr mentioned this pull request Jul 15, 2022
Merged
@fkiraly
Copy link
Collaborator

fkiraly commented Jul 17, 2022

@tobiasweede, FYI, the grid search has the best_params_ attribute etc only after fit has been called.

@tobias-weiss-ai-xr tobias-weiss-ai-xr force-pushed the InceptionTime branch 6 times, most recently from f467b38 to 22b8a01 Compare July 18, 2022 09:07
@tobias-weiss-ai-xr
[ENH] add inception time with example (from sktime-dl)
d906e40 
fix linting

apply suggested naming convention

comment grid search

remove tf hard dependecy

exclude inceptiontime from tests using pickle
fkiraly
fkiraly reviewed Jul 18, 2022
View reviewed changes
fkiraly
fkiraly reviewed Jul 18, 2022
View reviewed changes
@fkiraly
Copy link
Collaborator

fkiraly commented Jul 18, 2022

FYI, I do not consider the "refactor" comments as blocking.
What I would consider blocking is my suggestion to localize the specific checks as private, in the same file or module as the estimator, so we do not duplicate existing utilities modules. We can refactor to convert_to etc later. Or, you could refactor/replace now, as said the "refactor" part is not blocking.

@tobias-weiss-ai-xr
Copy link
Contributor Author

@fkiraly Thank you for you comments. I will work on the case and try to address the issues.

I also plan show up at a collab code session.
Probably not this Friday, but during the next weeks...

@fkiraly
Copy link
Collaborator

fkiraly commented Jul 18, 2022

I also plan show up at a collab code session.

Nice, looking forward to that!
Would be appreciated if you send a message on the slack contributors channel a few days ahead, so the people interested in neural networks know.

@AurumnPegasus AurumnPegasus mentioned this pull request Aug 26, 2022
Open
27 tasks
@achieveordie
Copy link
Collaborator

Hey, @tobiasweede Are you still working on this? We could really make use of InceptionTime in sktime. It'll be incredibly helpful, thanks!

@fkiraly
Copy link
Collaborator

fkiraly commented Apr 15, 2023

I've taken the freedom to wrap this up.
FYI @gaudium

fkiraly
fkiraly approved these changes Apr 16, 2023
View reviewed changes
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.

all fixed up now

@fkiraly fkiraly added the implementing algorithms Implementing algorithms, estimators, objects native to sktime label Apr 16, 2023
@fkiraly fkiraly merged commit a33378e into sktime:main Apr 16, 2023
@fkiraly fkiraly changed the title [ENH] add inception time with example (from sktime-dl) [ENH] migrate InceptionTime classifier and example (from sktime-dl) Apr 16, 2023
@phylopylo
Copy link

phylopylo commented Apr 17, 2023 •
edited
Loading

I have tested the InceptionTimeClassifier and confirmed that it runs without error.

Model took 48 minutes to train.

I am @gaudium on the discord, btw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fkiraly fkiraly fkiraly approved these changes

@aiwalter aiwalter Awaiting requested review from aiwalter

Assignees
No one assigned
Labels
implementing algorithms Implementing algorithms, estimators, objects native to sktime module:classification classification module: time series classification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tobias-weiss-ai-xr @fkiraly @TonyBagnall @achieveordie @phylopylo