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] First transfer of deep learning classifiers and regressors from sktime-dl #2447

Merged
merged 56 commits into from
Jun 1, 2022

Conversation

TonyBagnall
Copy link
Contributor

@TonyBagnall TonyBagnall commented Apr 11, 2022

this is my first go at this and open to discussion as to structure. Once we have agreed how to do CNN, the rest of the algorithms will come over quickly. However, there are features here some may dislike, so want to out there early. Points to note

  1. Introduces a whole new module called networks. This contains the network structures that are shared by both classifiers and regressors. This is necessary because it was agreed long ago to split the algorithms into regressors/classifiers (rather than have one class with both capabilities, which was my preferred structure)
  2. classifiers go in classification/deep_learning, and there is an abstract class there that inherits from BaseClassifier. This intermediate class is there because we can define default DL classifier predict_proba behaviour, and we want an extra abstract method.

@TonyBagnall TonyBagnall added the module:classification classification module: time series classification label Apr 11, 2022
@lmmentel lmmentel added the enhancement Adding new functionality label Apr 12, 2022
@TonyBagnall TonyBagnall marked this pull request as ready for review April 16, 2022 06:04
@TonyBagnall
Copy link
Contributor Author

Looks ok, but you need to adhere to the dependency handling via checks, see docs for that.

tried _check_dl_dependencies but it doesn't have a severity argument, so reverted back to _check_soft_dependencies. I'm not sure why there is a separate method for dl dependencies, will surely just be duplication of code, and I know how much you hate that

@fkiraly
Copy link
Collaborator

fkiraly commented May 11, 2022

tried _check_dl_dependencies but it doesn't have a severity argument

now it does: #2627

so reverted back to _check_soft_dependencies.

would in-principle be fine by me

I'm not sure why there is a separate method for dl dependencies,

more informative (deep learning specific) error message and developer comfort for an anticipated frequent developer case.

will surely just be duplication of code, and I know how much you hate tha

Agree, could easily be refactored.
Although in this case I would put user and developer comfort above "neat code".

@TonyBagnall
Copy link
Contributor Author

@fkiraly I believe you have someone working on the porting of sktime-dl, which is great. I would like to get this PR in first in order to fix the structure. I've lost track of what you wanted changing here, are you still blocking it, and if so, what do you want changing?

@fkiraly
Copy link
Collaborator

fkiraly commented May 27, 2022

@TonyBagnall, what have you done/decided about the dependency handling?

Also, it's not "me having someone work" on DL. sktime, as a programme & community, is hosting summer projects, one of which is working on this, yes. You are more than welcome to engage with the summer mentees, the meetings are on our calendar, and as CC member you also have a full view of the process (see "updates on GSoC" items).

@TonyBagnall
Copy link
Contributor Author

@TonyBagnall, what have you done/decided about the dependency handling?

Also, it's not "me having someone work" on DL. sktime, as a programme & community, is hosting summer projects, one of which is working on this, yes. You are more than welcome to engage with the summer mentees, the meetings are on our calendar, and as CC member you also have a full view of the process (see "updates on GSoC" items).

ok, soz my wording offends you, not sure what you mean by "decided about the dependency handling", could you clarify?

@fkiraly
Copy link
Collaborator

fkiraly commented May 29, 2022

ok, soz my wording offends you,

I'm not offended, I just wanted to disagree with the (possibly unintentionally) possessive framing.

not sure what you mean by "decided about the dependency handling", could you clarify?

Refers to my change request, in which I said:

Looks ok, but you need to adhere to the dependency handling via checks, see docs for that.

More precisely, I was asking above, for:

  • adding informative warning and error messages for developers using the DL functionality
  • removing any errors that would be triggered by module import

and with my more recent question, I have asked you for explaining whether and how this has been addressed.

Addressing could mean (a) agreeing with my request and taking some action, or (b) disagreeing and providing a statement why you think you do not need to do it, or (c) thinking something different needs to be done, taking alternative action.

In either case, I would kindly ask for explanation of the action taken and/or rationale.

@TonyBagnall
Copy link
Contributor Author

I have switched to your _check_dl_dependencies given you went to the trouble of updating it. I think it already complied with the docs on new soft dependencies (warn/error, imports in functions)

@TonyBagnall
Copy link
Contributor Author

@fkiraly I would like this in before the next release, could you unblock or give specific changes you would like?

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 good.

@xxl4tomxu98
Copy link
Contributor

is this resolved? e.g. am I able to install the sktime[all_extra] and then all soft dependency are satisfied? including tensorflow?

@xxl4tomxu98
Copy link
Contributor

The problem with sktime-dl is that I am using ARM based Mac
and it won't work when I try to do "conda install sktime-dl"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding new functionality module:classification classification module: time series classification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants