Skip to content

[ENH] Add LSTM-FCN estimator from sktime-dl #3714

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 20 commits into from
Dec 22, 2022
Merged

[ENH] Add LSTM-FCN estimator from sktime-dl #3714

merged 20 commits into from
Dec 22, 2022

Conversation

solen0id
Copy link
Contributor

@solen0id solen0id commented Nov 6, 2022 •
edited
Loading

What does this implement/fix? Explain your changes.

Add LSTM-FCN estimator from sktime-dl

I am using sktime & sktime-dl in university as part of a course project this semester. I would love to use the LSTM-FCN model from sktime-dl in my project, but since sktime-dl comes with some pretty old (and hard-pinned) dependencies, I decided to try and port the model over to sktime.

I made some very small changes to parameter names to stay in line with the existing models, such as the CNNClassifier (e.g changed nb_epochs to n_epochs).

Added jupyter notebook 02b_classification_multivariate_lstmfcn.ipynb with a small demo based on the CNN example.

Ran pytest -k LSTMFCNClassifier without errors.

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

No

What should a reviewer concentrate their feedback on?

Please focus on everything, except the network architecture itself. I did not implement it, all credit goes to Jack Russon from sktime-dl. Source.

I am happy to incorporate any other feedback you might have!

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
  • I've added unit tests and made sure they pass locally.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.
For new estimators
  • I've added the estimator to the online documentation.
  • I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

@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 Nov 6, 2022

Thanks!

Your linting is failing, kindly check code formatting, here is the guide:
https://www.sktime.org/en/stable/developer_guide/coding_standards.html

@solen0id
Copy link
Contributor Author

solen0id commented Nov 7, 2022

Hey @fkiraly, thanks for the initial review!
I ran the pre-commit suite a bunch of times and fixed up all errors that popped up, let me know what you think.

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

Looks overall reasonable!

The remaining issue is to isolate the soft dependency tensorflow. Please use the tag as stated above (in both classes), and all should be fine!

@solen0id
Copy link
Contributor Author

solen0id commented Nov 7, 2022

All done, let me know if the suggested author attribution works for you

@solen0id solen0id requested review from fkiraly and removed request for aiwalter and GuzalBulatova November 7, 2022 20:43
@solen0id
Copy link
Contributor Author

solen0id commented Nov 7, 2022

I replaced the actual grid search in the example notebook with a comment about grid search compatibility to speed up things

@solen0id
Copy link
Contributor Author

solen0id commented Nov 8, 2022

Hey @fkiraly, I added Jack and myself to the contributors file. Let me know if there is anything else I should fix up :)

@solen0id
Copy link
Contributor Author

I moved the AttentionLSTM class into the networks package, that should fix the failing soft dependency test 🤞

@solen0id
Copy link
Contributor Author

Hey @fkiraly, I merged the latest changes from upstream to resolve the merge conflict in .all_contributorsrc that just appeared. Would you kindly authorise another execution of the test suite please? Thanks 🙏

@solen0id solen0id force-pushed the add-lstm-fcn-from-sktime-dl branch from 63ccbe6 to 60f732f Compare November 18, 2022 11:03
@solen0id
Copy link
Contributor Author

Hey @fkiraly looks like there is a test failing, although it seems unrelated to any of my changes. Do you have an idea whats going on here?

FAILED sktime/classification/tests/test_base.py::test_deep_estimator_full[keras-adamax] - ValueError: `optimizer` of type <class 'keras.optimizers.optimizer_v2.adamax.Adamax'> cannot be serialized, it should either be absent/None/str/tf.keras.optimizers.Optimizer object

@solen0id
Copy link
Contributor Author

I pulled in your changes for #3817, that should hopefully do it now 🤞

@solen0id
Copy link
Contributor Author

Can we do another test run @fkiraly?

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 22, 2022

Of course! Sorry for being silent on this. Thanks for your contribution!

@patrickzib patrickzib added implementing algorithms Implementing algorithms, estimators, objects native to sktime module:classification classification module: time series classification labels Nov 24, 2022
@solen0id
Copy link
Contributor Author

All good, no need to apologize :)

I see there are still some OS + Python version combinations causing errors in the test matrix, I'll try to figure out what's wrong there

@solen0id solen0id force-pushed the add-lstm-fcn-from-sktime-dl branch from 48c4e25 to 7b40d21 Compare November 28, 2022 16:34
@solen0id solen0id force-pushed the add-lstm-fcn-from-sktime-dl branch from ecdd58f to df79e97 Compare November 28, 2022 16:40
fkiraly
fkiraly requested changes Dec 1, 2022
View reviewed changes
Copy link
Collaborator

@fkiraly fkiraly left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The tests failing are the ones commonly failing due to incomplete random seed setting in keras. This is a known issue and will be dealt with for all keras estimators simultaneously, if it will be dealt with.

So, you should add the tests as exceptions to tests._config, similar to the other keras based estimators.

@solen0id
Copy link
Contributor Author

solen0id commented Dec 2, 2022

Good stuff, thanks for the hint 👍

@solen0id solen0id requested a review from fkiraly December 5, 2022 11:04
@solen0id
Copy link
Contributor Author

solen0id commented Dec 9, 2022

Shall we give the tests another go @fkiraly ? :)

@solen0id
Copy link
Contributor Author

Looks like a compiler error occured during install of dependencies on macOS, looks to me like it might have just been a fluke. Do you agree @fkiraly? Maybe we can just re-run the test suite, everything else looks fine now

@solen0id
Copy link
Contributor Author

Hey @fkiraly, could you please authorize another run of the test suite? Thank you!

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 19, 2022

oops, sorry - did miss this one. Of course, let's see

@fkiraly fkiraly merged commit 878672f into sktime:main Dec 22, 2022
This was referenced May 23, 2023
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fkiraly fkiraly Awaiting requested review from fkiraly fkiraly is a code owner

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.
3 participants
@solen0id @fkiraly @patrickzib