search

mlr-org / mlr3learners

Recommended learners for mlr3
https://mlr3learners.mlr-org.com
GNU Lesser General Public License v3.0
89 stars 14 forks source link

Remove Custom mlr3 defaults section #240

Closed sebffischer closed 2 years ago

sebffischer commented 2 years ago

Sometimes we introduce new parameters and this is mentioned in the "Custom mlr3 defaults" section, which is a misleading name

pat-s commented 2 years ago

"Custom mlr3 defaults" might not be the best wording but I actually like it more than "Parameter Changes" as the latter is quite undescriptive and general. As a user, I would not know what this means in this context.

Maybe something along the lines of "mlr3 parameter defaults which differ compared to upstream package" or similar?

sebffischer commented 2 years ago

My whole point is that the focus on "defaults" is wrong. I don't care about the wording, as long as it is semantically correct ,which it currently isn't (or the section is being misused).

pat-s commented 2 years ago

Why would "defaults" be wrong in this context? "defaults" means essentially "the value present by default if the user would run the function as is".

And given that we have set/altered these from the upstream learner, we named it "custom mlr3 defaults".

sebffischer commented 2 years ago

E.g. in ranger, we introduce a new parameter "mtry.ratio".

sebffischer commented 2 years ago

This is what I meant in my initial explanation of the PR: "Sometimes we introduce new parameters ..."

This also happens in extralearners

pat-s commented 2 years ago

E.g. in ranger, we introduce a new parameter "mtry.ratio".

Yes, but these cases are an expection and not the rule :)

You're definitely right that we might want to (should!) highlight such cases in some way (either in a new section or in some other way). However, the general wording for the other cases in which we just change some upstream learner defaults for some good reason would still fit, wouldn't it?

This is what I meant in my initial explanation of the PR: "Sometimes we introduce new parameters ..."

👍 This is a good reasoning, however we should not apply a general change for certain expections I'd say but rather highlight the latter? What do you think?

sebffischer commented 2 years ago

Ok, I suggest adding a section "Custom mlr3 parameters"

mllg commented 2 years ago

Custom mlr3 parameters works for me.

Do you agree with this change @RaphaelS1?

RaphaelS1 commented 2 years ago

Yup sounds good

pat-s commented 2 years ago

While looking at it, I had the feeling that the difference between

might not be fully clear for users on the first look. How about changing "Custom mlr3 parameters" to "additional parameters added by mlr3"?

sebffischer commented 2 years ago

Sometimes parameters are renamed (RWeka in extralearners) and this is currently also put in the "Custom mlr3 parameters". I think having it a little bit more generic is fine, otherwise we need too many sections.

pat-s commented 2 years ago

I was asking for a renaming of the section, not adding a third one. Was this a misunderstanding?

Maybe then "additional/renamed parameters by mlr3"?

sebffischer commented 2 years ago

The section is currently used to:

I think the way it is now is good, all parameters also have additional explanations why they are listed in the respective section, so people should be smart enough to figure it out.

sebffischer commented 2 years ago

Also we already had 4 people agreeing on this so I don't think this needs any further discussion tbh

pat-s commented 2 years ago

It's an important change, worth discussing even multiple times imo.

You wanted to rename the section to "Additional parameters added by mlr3", which excludes the use for point 1.This would then require a new section, because I started this whole PR because of wrong section names. So this was our misunderstanding.

Yes and no, that's why my last comment updated the wording again. So it would still stay at 2 different sections.

I think the way it is now is good, all parameters also have additional explanations why they are listed in the respective section, so people should be smart enough to figure it out.

Fair enough, just wanted to help ;)

so I don't think this needs any further discussion tbh

Didn't wanted to offend you or sound nagging.

sebffischer commented 2 years ago

Ups I hope I did not sound annoyed / offended but I just want to get done with this PR. If you think it is important, feel free to take over and adjust it

pat-s commented 2 years ago

Ups I hope I did not sound annoyed / offended but I just want to get done with this PR. If you think it is important, feel free to take over and adjust it

Sounded a bit so, yes :)

It's not about taking over a PR/work item - some discussions just need some time and follow-ups sometimes, especially when its about wording.

Thanks for raising the point in the first place, please go ahead!

sebffischer commented 2 years ago

Ok, I will be more patient next time! It is just that I already changed in twice in mlr3extralearners - probably I should have waited with that but I didn't...