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

Update support of `getClassWeightParam()` #891

Merged
merged 14 commits into from May 28, 2019

Conversation

Projects
None yet
7 participants
@ja-thomas
Copy link
Member

commented May 13, 2016

This should fix #525, if we merge PR #883 before this, the check for class.weights can be done right (see FIXME).

@larskotthoff

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

Does this depend on #883 or should the tests pass?

@ja-thomas

This comment has been minimized.

Copy link
Member Author

commented May 13, 2016

no, I think I did something wrong with the tests, I'll check.

@berndbischl

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

@ja-thomas
did you run the respective tests locally before you pushed?

@berndbischl

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2016

@ja-thomas
this seems like a mini issue: can you please get back here soon?

@berndbischl

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2016

@ja-thomas

Please always think about API changes. the docs of getClassWeightParam say that you return a single numeric Param object. For your getClassWeightParam.BaseEnsemble you now return a list of params.

This is neither documented nor mentioned here in the thread....

@berndbischl

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2016

The other thing is that in principle we wanna avoid this. Because now the function API does not tell us what type is returned. which is usually not good. But I also dont really see how to make this better?

But lets think about this.

@berndbischl

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2016

Can we maybe get away without returning that param? Or simply being allowed to ask for it for a base learner? I mean all other operation like set it and get it should be abstracted away through API?

@ja-thomas

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2016

@berndbischl I think we talked about how to handle ensembles (But that was some time ago).

So for wrapper and learner this shouldn't be a problem. For ensembles I don't see another possibility, either return all possible base weights in a list or give a meaningful error? I haven't thought about the API change, so the list may be the worst option...

Or maybe we add a new optional argument to getWeightParam() to select an element in the ensemble from which to get the weight param?

@berndbischl berndbischl added this to the v2.10 milestone Jun 14, 2016

@giuseppec giuseppec removed this from the v2.10 milestone Aug 8, 2016

@ja-thomas ja-thomas added the prio-low label Aug 8, 2016

@larskotthoff

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2017

What's the status here?

@ja-thomas ja-thomas force-pushed the fix_525_getClassWeightParam branch from 68675af to 162c036 Mar 10, 2017

ja-thomas added some commits Mar 10, 2017

@ja-thomas ja-thomas requested a review from larskotthoff Mar 10, 2017

@berndbischl berndbischl force-pushed the master branch 2 times, most recently from cf3db65 to 57f099e Mar 3, 2018

@ja-thomas ja-thomas force-pushed the master branch from 57f099e to 20d5a39 Mar 3, 2018

@pat-s

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

@ja-thomas Is this PR still valid?

pat-s added some commits Apr 8, 2019

@pat-s pat-s changed the title Fix #525 get class weight param Update support of `getClassWeightParam()` May 20, 2019

pat-s added some commits May 23, 2019

@pat-s pat-s merged commit 9f0f5cf into master May 28, 2019

1 check passed

deploy/netlify Deploy preview ready!
Details

@pat-s pat-s deleted the fix_525_getClassWeightParam branch May 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.