-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
# Conflicts: # sktime/_contrib/distance_refactor.py # sktime/_contrib/tests/test_data_io.py # sktime/_contrib/tests/test_experiments.py # sktime/_contrib/timing_comparisons.py
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 |
now it does: #2627
would in-principle be fine by me
more informative (deep learning specific) error message and developer comfort for an anticipated frequent developer case.
Agree, could easily be refactored. |
@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? |
@TonyBagnall, what have you done/decided about the dependency handling? Also, it's not "me having someone work" on DL. |
ok, soz my wording offends you, not sure what you mean by "decided about the dependency handling", could you clarify? |
I'm not offended, I just wanted to disagree with the (possibly unintentionally) possessive framing.
Refers to my change request, in which I said:
More precisely, I was asking above, for:
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. |
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) |
@fkiraly I would like this in before the next release, could you unblock or give specific changes you would like? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good.
is this resolved? e.g. am I able to install the sktime[all_extra] and then all soft dependency are satisfied? including tensorflow? |
The problem with sktime-dl is that I am using ARM based Mac |
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