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

Compatibility with sktime v0.6.1 #87

Merged
merged 11 commits into from Jun 17, 2021

Conversation

Riyabelle25
Copy link
Contributor

@Riyabelle25 Riyabelle25 commented Jun 8, 2021

Reference Issues/PRs

Fixes #74 , #76

What does this implement/fix? Explain your changes.

This PR fixes issues #74 and #76 by updating sktime references in the code to that of sktime v0.6.1. Hence this resolves compatibility issues with sktime v0.6.1, bringing sktime_dl up-to-date with the latest version of sktime.
It also updates deprecated Keras code to resolve some of the build failures on running Pytest.

Tested by:

conda activate venv
git clone https://github.com/sktime/sktime-dl.git
cd sktime-dl
pip install  .
python
>> from sktime_dl.deeplearning import CNNClassifier

After this I ran pytest in the root directory, and confirmed that no tests are failing due to incompatibility with sktime.
Rather they are associated with version incompatibilities of Keras, Tensorflow, and Numpy, and I debugged by updating deprecated Keras code.

@TonyBagnall TonyBagnall requested a review from mloning June 9, 2021 09:44
@TonyBagnall
Copy link
Contributor

hi, thanks for this, the travis checks are not currently working so dont worry too much about the errors. @mloning could you review? I'll ask @ABostrom to take a look too

@Riyabelle25
Copy link
Contributor Author

Hi, thanks! Currently, there's just the one test failing because of Keras.

@mloning
Copy link
Contributor

mloning commented Jun 11, 2021

@Riyabelle25 as discussed, I suggest adding the pytest decorator to see if this test fails for all networks or only one of them.

@ABostrom
Copy link
Contributor

Going to go through this, today with @jnrusson1, as I'd like to get this PR'd in before dev days next week.

Really great job @Riyabelle25 looks decent.

@ABostrom
Copy link
Contributor

@jnrusson1 noticed a issue and raise this in the #87 issue.

I haven't had a chance to verify this, but @Riyabelle25 if you wouldn't mind taking a closer look that would be very helpful :)

@Riyabelle25
Copy link
Contributor Author

I've fixed linting and parametrized the networks to see which fail. Thanks, @ABostrom, @jnrusson1!

@Riyabelle25
Copy link
Contributor Author

2021-06-17T07:53:40.4365930Z It seems that sktime cannot be built with OpenMP support.
Buildwheels for sktime on macOS build check failed 😲
Has this happened before?

@mloning
Copy link
Contributor

mloning commented Jun 17, 2021

@Riyabelle25 it shouldn't be necessary to compile sktime as we provide precompiled wheels for it, not sure why it tries to build it from source.

Copy link
Contributor

@TonyBagnall TonyBagnall left a comment

Choose a reason for hiding this comment

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

I'll try this out

@TonyBagnall TonyBagnall merged commit 4225146 into sktime:master Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Problem with sktime-dl installation; cannot import e.g. sktime_dl.deeplearning
5 participants