Skip to content

Conversation

glemaitre
Copy link
Member

No description provided.

@coveralls
Copy link

coveralls commented Jul 27, 2016

Coverage Status

Coverage decreased (-1.3%) to 98.042% when pulling ff52bfc on glemaitre:issue_111 into 0c46346 on scikit-learn-contrib:master.

@glemaitre
Copy link
Member Author

The decrease of the coverage come from disabling the unit test for the CNN method

@glemaitre glemaitre changed the title [WIP] Issue #111 - Handle multiclass/binary class target [MRG] Issue #111 - Handle multiclass/binary class target Jul 27, 2016
@glemaitre
Copy link
Member Author

@dvro @chkoar If you can have a look to the changes. It should be ready for a merging normally.

@chkoar
Copy link
Member

chkoar commented Jul 27, 2016

In order to avoid code and docsting duplication I would define a new private class _BinarySamplerMixin that it will inherit SamplerMixin and I would change the fit there. Then all binary samplers should inherit from that class.

@glemaitre
Copy link
Member Author

One or two classes: BinarySamplerMixin and MulticlassSamplerMixin?

@chkoar
Copy link
Member

chkoar commented Jul 27, 2016

At first I thought to split the SamplerMixin in two classes like you said. But we want something temporarily just to pass the check_estimator test thats why I said a private class.

@coveralls
Copy link

coveralls commented Jul 27, 2016

Coverage Status

Coverage decreased (-1.3%) to 98.005% when pulling e7ac93f on glemaitre:issue_111 into 0c46346 on scikit-learn-contrib:master.

@coveralls
Copy link

coveralls commented Jul 27, 2016

Coverage Status

Coverage decreased (-1.3%) to 98.005% when pulling 60d8dbc on glemaitre:issue_111 into 0c46346 on scikit-learn-contrib:master.

@glemaitre
Copy link
Member Author

@dvro @chkoar Good for another round

@chkoar
Copy link
Member

chkoar commented Jul 27, 2016

Finally, will we follow auto-sklearn convention?
Wouldn't it be better to name it _estimator_properties?
Also, do we need to make get_properties public? I guess so.

@coveralls
Copy link

coveralls commented Jul 27, 2016

Coverage Status

Coverage decreased (-1.4%) to 97.926% when pulling 012da67 on glemaitre:issue_111 into 0c46346 on scikit-learn-contrib:master.

@coveralls
Copy link

coveralls commented Jul 27, 2016

Coverage Status

Coverage decreased (-1.4%) to 97.926% when pulling 012da67 on glemaitre:issue_111 into 0c46346 on scikit-learn-contrib:master.

imblearn/base.py Outdated
"""Get the properties for this estimator."""

return cls._estimator_prop
class BaseBinaryclassSampler(six.with_metaclass(ABCMeta, SamplerMixin)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would name it BaseBinarySampler

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

@coveralls
Copy link

coveralls commented Jul 27, 2016

Coverage Status

Coverage decreased (-1.3%) to 97.986% when pulling e9d8f1a on glemaitre:issue_111 into 0c46346 on scikit-learn-contrib:master.

@glemaitre
Copy link
Member Author

@chkoar I made the changes

@chkoar chkoar merged commit b26be15 into scikit-learn-contrib:master Jul 29, 2016
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.

4 participants