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

add custom regressor classifer #1186 #1191

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ankitrajixr
Copy link

@ankitrajixr ankitrajixr commented Mar 17, 2021

#1186

What does this PR do?

We found certain regressors and classifiers not included in the model selection of the machine learning pipeline. Hence, we have included a few.

Contributors are:
Ankit Raj https://github.com/ankitrajixr
Darshan Chandra https://github.com/drshnchndr
Sindhu Subramanya https://github.com/Sindhu-Subramanya

@JDRomano2
Copy link
Contributor

Hi @ankitrajixr , thanks for submitting this pull request. There are a couple of issues here:

  1. The biggest one is that the tests fail. AppVeyor reports 10 errors, which look like syntax errors. Please also note that you can't put new import statements in the config dictionary files. Every tunable metaparameter must be a predefined property of the operator.
  2. Once tests pass, we will need to be sure that code coverage metrics do not drop. It may be necessary to write new tests to make sure we have adequate code coverage with the test suite.
  3. Before we can merge these changes we need to have some indication of their performance. This could be as simple as showing training times with and without the new operators included, and an indication of how often the new operators find their way into the final pipelines.

@JDRomano2
Copy link
Contributor

@ankitrajixr as another note, please also make sure to include detail describing the exact nature of the changes this pull request (when completed) will introduce. For reference, you may look at the following blog post: https://github.blog/2015-01-21-how-to-write-the-perfect-pull-request/

@rachitk
Copy link

rachitk commented Jul 30, 2021

Note that this PR will fail to pass checks and the proposed regressor will not function due to its use of the kernels within the configuration dictionary, which will cause TPOT to crash when trying to evaluate any pipeline string with these kernels due to them not existing in TPOT's operator space as TPOT does not consider parameters when importing operators.

#1218 touches on some of the changes that would need to be made to TPOT to support these kind of configurations (needing to either allow the user to pass references to objects used as parameters when instantiating TPOT or finding a way to dynamically assign those references within TPOT itself). Until those changes are made, I don't think this PR can be merged quite yet.

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.

3 participants