Skip to content

Conversation

achieveordie
Copy link
Collaborator

Reference Issues/PRs

Fixes #3816

What does this implement/fix? Explain your changes.

Replacing keras.optimizers with tf.keras.optimizers along with few other API-related changes. Also reformatted save to check for model presence first, must have gone unnoticed in the original PR.

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

No

What should a reviewer concentrate their feedback on?

Any other comments?

Tested for the latest tensorflow version (2.11.0)

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.

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.

The fix to the test looks good.

Regarding the change to the save method - should that not also simply work with an unfitted object? I thought there's no harm in that, there may be an use case.

@achieveordie
Copy link
Collaborator Author

We're testing for the presence of the model (self.model_ is not None should evaluate to True) as per the current implementation. This, in my opinion, is implicit in the documentation we have built during serialization (by mentioning that keras/ subdirectory is present in the zipped file).

I also have trouble thinking of a use case for saving an estimator that lacks the most important element. Perhaps I've probably used the wrong kind of error (NotFittedError) to give you a false impression (I probably did that since there was no NotBuiltError?).

In any case, I'm making a commit that actually lets you save the estimator even when self.model_ = None for DL Classifiers. Let's discuss if this is actually what we want so we can change the documentation/other parts of the code.

@achieveordie
Copy link
Collaborator Author

I don't fully understand why the current failure (in 3.10, macOS-11) is occurring. I also highly doubt this failure has anything to do with the current PR's work since the problem is setting up sktime.

One possibly relevant piece of information is that grpcio is downloaded directly as a wheel in Windows whereas it is downloaded as a .tar.gz file for Mac and it is during the building stage of this package where the error is occurring.

fkiraly
fkiraly previously approved these changes Nov 29, 2022
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.

Seems to work now, thanks!

The failure seems to have been sporadic.

As said, I would remove the raised exception in save and ensure it works for unfitted models, but it is not a blocker since that behaviour is already on main.

@fkiraly fkiraly merged commit e4fa178 into sktime:main Nov 30, 2022
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] testing deep learning base class fails due to optimizer type
2 participants