-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Adding support for Neural Network Embedding #895
base: master
Are you sure you want to change the base?
Adding support for Neural Network Embedding #895
Conversation
except ImportError: | ||
tf = None | ||
if tf is None: | ||
return |
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.
Do you guys prefer raising a SkipTest here or leaving it like this?
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.
SkipTest should be OK since TF should be a optional dependency.
X_transformed = np.hstack( | ||
(self._embedding_mlp(self.estimator, X), X_transformed) | ||
) | ||
else: |
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.
Chose not to do checking here otherwise you'll need to do an import at the top; which adds whatever flavour of the month neural network library to be a hard dependency to this. I don't have better ideas at this point but happy for any suggestions
self: object | ||
Returns a copy of the estimator | ||
""" | ||
if not issubclass(self.estimator.__class__, MLPClassifier) and not issubclass( |
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.
- not sure if this is the best way to check for Keras etc.
Q: how would we support pure tensorflow or pytorch? I've used Keras here to try to use the Keras scikit-learn wrapper
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.
Thank you for this PR. Keras scikit-learn wrapper is good so far.
is this still happening? |
Up to the maintainers |
I am sorry for late response due to out of office for more than one month. I think removing |
[please review the Contribution Guidelines prior to submitting your pull request. go ahead and delete this line if you've already reviewed said guidelines.]
What does this PR do?
Attempts to resolve #809
Where should the reviewer start?
Have a look at
tpot.builtins.embedding_estimator
How should this PR be tested?
It has been tested on Python 3.7 on two environments; one with
tensorflow 2.0.0a
installed and one withouttensorflow
installed at all. It was tested via:Any background context you want to provide?
See also #809 - as requested. I've made an attempt to ensure the API is compliant and sensible
What are the relevant issues?
#809
Screenshots (if appropriate)
Questions: