tlverse / sl3

💪 🤔 Modern Super Learning with Machine Learning Pipelines
https://tlverse.org/sl3/
GNU General Public License v3.0
100 stars 40 forks source link

Discuss: Naming learner R6 classes and .R files #30

Closed osofr closed 7 years ago

osofr commented 7 years ago

Can we reverse the current naming scheme from h2o_GLM_Learner to Learner_h2o_GLM?

That way all learner R files will be sorted "together" and will be easily findable.

Just a suggestion. Can we discuss pros and cons of different naming approaches here?

jeremyrcoyle commented 7 years ago

Seems like a good idea to me. @podTockom and @nhejazi: thoughts?

nhejazi commented 7 years ago

Yes - better sorting order is always good. I like it!

imalenica commented 7 years ago

I like it as well.

jeremyrcoyle commented 7 years ago

Working on the refactor now. A related issue is how to distinguish between SuperLearner the algorithm and SuperLearner the package (and also sl3 the package). Any good ideas there?

nhejazi commented 7 years ago

I think we should call the algorithm itself simple SuperLearner (SL for short if we want). For the packages (especially old Super Learner), we could just append pkg to the beginning or end of the name?

jeremyrcoyle commented 7 years ago

So the Learner that uses a SuperLearner package wrapper would be Learner_pkgSuperLearner?

jeremyrcoyle commented 7 years ago

Other thoughts:

  1. Maybe the prefix to Learners should be something with fewer characters than Learner?
  2. Does it still make sense to call this package sl3 since the scope is a bit larger than SuperLearner at this point?
nhejazi commented 7 years ago

Yea, I think Learner_pkgSuperLearner or Learner_pkg_SuperLearner are fine.

  1. The prefix is mostly an organizational point, right? It doesn't really matter how long the prefix is since I assume we're all just tab-completing to get the files we want -- is that what you meant?

  2. I think for changes of this kind (name, scope, etc.), we should have a short discussion. In my view, our core goal is still to re-implement the Super Learner algorithm (and get it a bit more right this time, now that we have ~10yrs of hindsight and better tools like R6 to work with), with the goal being for this re-design to offer a lot to new and existing TMLE implementations (and also w/e else people want SL for of course). If we see this project expanding to fill a bigger role (a better library than mlr, in an organizational sense), we should discuss that and adapt accordingly

jeremyrcoyle commented 7 years ago

Regarding the prefix, I'm just over here doing the refactor and getting real tired of typing Learner when lrnr or something would do (compare the SL. prefix in SuperLearner and the regr. and classif. prefixes in mlr). I know tab completion solves this problem a lot of the time, but as a relatively slow/lazy typist, I get annoyed at unnecessarily long variable names (I'm looking at you java: https://docs.oracle.com/javase/8/docs/api/java/beans/beancontext/BeanContextServiceProviderBeanInfo.html)

osofr commented 7 years ago

Lets do lrnr_ prefix.

Also, either snake-style or underscore naming, but not both please!

osofr commented 7 years ago

Does it mean that your refactor will put a conflict on merge on my changes? I added grid-based h2o last night.

jeremyrcoyle commented 7 years ago

What's snake-style? Also, it will almost certainly conflict, but if you get your PR in first, it'll be on me to fix the merge conflict 😛

nhejazi commented 7 years ago

Oh, yea, lrnr sounds good to me (I guess I just wasn't sure what the alternatives were)

@osofr -- I had proposed use of snake_case in keeping with tidyverse style. Also not sure what snake-style naming is

osofr commented 7 years ago

lrnr_MyFavorite

osofr commented 7 years ago

I guess I was referring to UpperLower case, it must be called something else and I am wrong.

jeremyrcoyle commented 7 years ago

I think generally tradition is that class defintions start with uppercase names and class objects start with lowercase names to distinguish them. Of course, we can break with tradition on that

nhejazi commented 7 years ago

I think what's now been proposed lrnr_SomethingNew is some combination of snake case and camel case (guys, we can invent our own case!)

Is the current suggestion that we use lrnr_Classdef and lrnr_objectdef? This sees reasonable (and tidy!) to me, but now sure what current thinking is

jeremyrcoyle commented 7 years ago

Hard to say I guess. Definitely we should try to eliminate camelCase with prejudice. How about: lrnr_a_learner=Lrnr_a_learner$new() ?

osofr commented 7 years ago

I agree with last suggestion.

jeremyrcoyle commented 7 years ago

Fixed!