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

predict_types should be an active binding #851

Closed
pfistfl opened this issue Aug 24, 2022 · 4 comments
Closed

predict_types should be an active binding #851

pfistfl opened this issue Aug 24, 2022 · 4 comments
Assignees
Labels

Comments

@pfistfl
Copy link
Member

pfistfl commented Aug 24, 2022

Since I am stumbling over this for the nth time:

predict_type and predict_types are easy to confuse (and it happens to me quite a lot).

  • predict_type: The concrete type of the prediction the learner should yield
  • predict_types: The theoretical capabilities of the learner: Which types of prediction can it yield.
    I think for less involved users, this might be an even bigger problem.

Solution:

predict_types should IMHO be immutable (this is a property of the learner).
-> Encode as an AB and if the user tries to set it point her/him to predict_type in the error message

More generally: Do we need predict_type?

  • Is there a single learner that uses it during training?
  • predict_type is mutable after training so we can break learners if they were to use it
  • If we e.g. want to mutate predict_types after training for BenchmarkResults this is not possible, anymore due to the use of read-only AB's.
  • For most cases, it could just be an extra arg added to $predict instead.

Sidenote Default

It's super annoying (and IMHO unncessary) to set predict_type = "prob".
I always remember that when I try to use probabilistic measures AFTER having trained the model.
And in 99.9% of cases it does not matter what predict type was set for the inducer and I could change it post-hoc (which afaik works as long as the learner is not e.g. in a resample result where things are way more difficult).
Question: Should we not by default predict prob IF the learner can do it? Do we have any learners that can not predict prob?

@mb706
Copy link
Collaborator

mb706 commented Sep 26, 2022

Is there a single learner that uses it during training?

Grepping a bit:

I'd conclude that for the few cases where predict_type is used in training, it could be a hyperparameter -- for xgboost and earth it already is, in a way, since predict_type is only used here to check if the hyperparameters are set up correctly. I think we could therefore just use $predict_type during prediction (or as an argument of $predict(); or both, with the latter overriding the former), throwing errors during prediction in the few cases where different HPs were needed during training.

@pfistfl
Copy link
Member Author

pfistfl commented Oct 25, 2022

I guess to move forward here we would need the opinion of @berndbischl and @mllg?

@sebffischer sebffischer self-assigned this Aug 16, 2024
@sebffischer sebffischer removed their assignment Aug 17, 2024
@berndbischl
Copy link
Member

the reason why this was implementred are somewhat historic.
or rather: it is as @mb706 has said: some learners need a param to be changed before training, so that in predict you can ask for probs.

more importantly: this would now REALLY change very CENTRAL API. so I am very hesitant to do that.

what you write about read only AB seems fine to me

@be-marc be-marc self-assigned this Dec 20, 2024
@be-marc
Copy link
Member

be-marc commented Dec 20, 2024

Solved in #1233

@be-marc be-marc closed this as completed Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants