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

Remove default tuning objective and make it a required field. #381

Closed
desilinguist opened this issue Nov 1, 2017 · 15 comments
Closed

Remove default tuning objective and make it a required field. #381

desilinguist opened this issue Nov 1, 2017 · 15 comments
Assignees
Milestone

Comments

@desilinguist
Copy link
Member

Right now, the default metric for all learners is f1_score_micro which doesn't make sense for regressors.

(Note: This issue is related to but separate from #350.)

@desilinguist desilinguist self-assigned this Nov 1, 2017
@desilinguist desilinguist added this to the 1.5 milestone Nov 1, 2017
@desilinguist
Copy link
Member Author

I am not really sure how to do this honestly since the default objective is set during config parsing and to override the defaults means actually checking whether all the learners are regressors and doing so means actually instantiating them early during config parsing itself into objects to check their _estimator_type attribute. This seems wasteful to me.

I am trying to think of a better solution. Suggestions and thoughts welcome.

@aoifecahill
Copy link
Collaborator

Couldn't we do something a little less sophisticated and just hard code the string names of the regressors and classifiers wrapped by skll? We do that in other places for other reasons.

It wouldn't work for a custom learner though.

@desilinguist
Copy link
Member Author

So, then we have another place where we have to update the names of learners when we add new ones? I am not a fan of that approach - what if someone forgets to update the list? Then we need to add more tests for that scenario and basically increase the complexity of the codebase. Plus it wouldn't work for a custom learner like you said.

May be we should just not have a default objective at all? If you don't specify an objective in the config file, you get an early error during config parsing itself. This way, you don't get an incomplete experiment and the responsibility of the objective is on the user itself.

It would break backward compatibility though for old config files which is not ideal either.

@aoifecahill
Copy link
Collaborator

Well if we don't want to add complexity to the config reading by doing the checks there, then I think the next best thing is to not have a default, but I suppose that would have to be part of a 2.0 release if it's breaking backwards compatibility.

@desilinguist
Copy link
Member Author

Agreed. Let's do that in v2.0.

@stale
Copy link

stale bot commented Mar 6, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 6, 2018
@desilinguist
Copy link
Member Author

We will fix this in v2.0. Keep it open.

@stale stale bot removed the stale label Mar 6, 2018
@stale
Copy link

stale bot commented Jun 4, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 4, 2018
@desilinguist
Copy link
Member Author

Keep it open for v2.0

@stale stale bot removed the stale label Jun 4, 2018
@stale
Copy link

stale bot commented Sep 2, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 2, 2018
@aoifecahill
Copy link
Collaborator

keep it open

@stale stale bot removed the stale label Sep 6, 2018
@stale
Copy link

stale bot commented Dec 5, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 5, 2018
@desilinguist
Copy link
Member Author

keep it open please

@stale stale bot removed the stale label Dec 5, 2018
@desilinguist desilinguist added this to To do in SKLL Release v2.5 Feb 5, 2019
@desilinguist
Copy link
Member Author

As part of this issue, I am also going to get rid of the objective field and keep only the objectives field. This would simplify the code a lot and also improve consistency with the metrics option (both being lists of scoring functions).

@desilinguist desilinguist changed the title Change default metric to Pearson correlation for regressors Remove default tuning objective and make it a required field. Feb 7, 2019
@desilinguist desilinguist moved this from To do to In progress in SKLL Release v2.5 Feb 11, 2019
@desilinguist
Copy link
Member Author

Addressed by #458.

SKLL Release v2.5 automation moved this from In progress to Done Feb 13, 2019
@desilinguist desilinguist removed this from Done in SKLL Release v2.5 Sep 12, 2019
@desilinguist desilinguist added this to the v2.0 milestone Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

2 participants