Skip to content

MNT Refactor the three base scorers into _BaseScorer #14388

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Jul 17, 2019

This PR refactors the three base scorers (_PredictScorer, _ProbaScorer, _ThresholdScorer) into _BaseScorer. These are all private classes so we can remove them directly.
Honestly I'm not sure whether we want this (vote +0 for myself :))

@cmarmo cmarmo added Needs Decision Requires decision Needs Info labels Sep 18, 2020
Base automatically changed from master to main January 22, 2021 10:51
@jjerphan
Copy link
Member

Hi @qinhanmin2014,
are you still working on this?

@cmarmo cmarmo removed the Needs Info label Mar 9, 2021
@rth
Copy link
Member

rth commented Mar 9, 2021

@jjerphan If you are interested you can probably continue this is a separate PR.

@thomasjpfan
Copy link
Member

This PR is squarely on "needs decision". Single class with if statements or inheritance? For me, the answer comes down to the design that is to easier maintain.

(I would prefer a refactoring that uses composition, but that is my preferred design for OOP)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants