-
Notifications
You must be signed in to change notification settings - Fork 1.9k
TrainersName pattern (Discussion) #2762
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
Comments
Maybe |
What ModelName means? Can you give example? |
StochasticGradient-FieldAwareFactorizationMachine-Calibrated-BinaryClassification |
This is also related to #2172 . The trainer estimator Class names we finally decide upon should also match the corresponding MLContext names So if a user has to specify an SDCA binary trainer with advanced options, they would have to say something like : // Pipeline. It may be overly verbose. Thoughts ? |
Right. It's not even I think naming things calibrated or not is fine if there is actually a distinction. But, I would pick one to identify as being calibrated or uncalibrated. If a class of models always produces calibrated or always non-calibrated models, no need to call attention to it. In any case it will be obvious from the return value and the generic parameters.
I know we're asking for hard and fast rules, but I think this is one of those cases where you just have to accept that if having a uniform procedure for naming would lead to a ridiculous situations like identifier names greater than 80 characters, we need to accept that a slightly irregular situation is better than being perfectly regular. If it's any consolation, |
(thoughts from a non-data scientist):
|
Here's a proposal
EDITS Proposed:
|
That proposal looks good @abgoswam. Here are some questions/comments:
|
|
I don't have a huge preference. I was mostly just trying to get what the policy is. If we go by "This is the algo's standard name - ex. LightGBM", then I think that is a fine policy. |
FYI, the Proposed MLContext Names above, are 90+% what we already have today (#2522). MLContext Names pattern for acronyms is spelled out with the exception of LightGbm. I think MLContext names look good as is, but I'll be open to changing the acronym spelling rule, in favor of making the trainer names shorter. I mean StochasticDualCoordinateAscentNonCalibratedBinaryClassificationTrainer is just mind-numbing. Now that we have better documentation any developer can hover over the name and know what Sdca, Sgd, or Svm is. We also need to standardize the acronyms casing between ALL-CAPS and Pascal (SVM vs Svm). It's different for different acronyms and trainer vs mlcontext names. From user perspective it would be helpful to have the same <Trainer_API_Name> in both MLContext and Trainer class names below:
|
Our preference has been to rely on the "brand" name of something. E.g.: SymSGD is not a brand name, in fact, publicly it seems like they prefer the name "Parasail" and "Project Parade" if I look at the people behind the algorithm. So I might prefer SymbolicSGD here, since unlike light GBM this does not seem to be externally branded as we internally branded it, back during the initial collaboration. (As opposed to LightGBM, which was released with that name.) SVM and LBFGS are harder ones. These are names that are well understood in the community, to the point where it would possibly do more harm than good to spell them out (especially L-BFGS). I am pretty comfortable with SVM, but I'd also be comfortable spelling out "Support Vector Machine." It's short enough and clear enough. So, do as you will here. I am less comfortable with L-BFGS, but in both directions. I also feel like spelling that guy out would do more harm than good... If it comes down to it I suppose I can live with My opinions are not strong, just thought I'd register them though. |
The general guidance from a .NET API perspective is to use PascalCasing for acronyms over two letters, and for two-letter acronyms both letters are capitalized. See https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions. For |
Thanks folks for the comments. Based on the feedback I am listing down the learners where we will use acronyms (with PascalCasing for acronyms over two letters)
@shmoradims . As noted above there is a prefix match of the {AlgoName} between MLContext name and Class name. So we should be good there. Next I will go ahead and make the code changes as per the above table + additional suggested edits captured in this table |
We also plan to ship |
added RandomizedPcaTrainer to the above table (that includes the trainers with acronyms) Also listing down a few trainers missed above :
|
I am jumping in this late, but two thoughts: See what we get with the above proposal when we use them:
notice the the current:
looks a lot better.. I would leave the ex: not The examples scikit names are short too. |
@sfilipi, do you need |
I have a question: Are we discussing the names of the factory methods in |
Proposal above : #2762 (comment) @markusweimer . The proposal above discussed both "Proposed MLContext name" and "Proposed Class name" . @sfilipi . We already took into consideration that we do not need to repeat the info already present in the MLContext chain. Hence the tables above shows the "Proposed MLContext name" and "Proposed Class name" .
|
The usage of the underlying class, from the user prospective, will be from the actual extension of the mlContext, mostly to specify the options. In this context, the name trainer adds no info, and the task adds no info. Now because they are separate types, they need their names distict, so we need some sort of identifier for the trainers that we have for more than one task (SDCA, FastTree, LightGBM). For those cases we can keep the task in the name, for the others drop it.. To summarize, to avoid verbosity from the user prospective:
|
for the extensions, just the algorithm (LightGBM). My rationale is just.. keep them as short as it makes sense.. |
I thought we decided in #1318 to use the suffix:
|
@eerhardt when we were discussing #1318 the shape of the API was different; the Trainer names were not user facing, we had that delegate over the Arguments class that had a short name. The AlgorithmNameTrainer was mostly for our codebase uniformity and documentation, but didn't appear in the API as much, now it does. |
Based on the discussion above, here is the convention we are following for trainer estimator names.
Based on the above, here is the list of trainer estimator names we expose for V1.0
Trainers estimators where we feel it is OK to use acronyms
Base classes
|
I would instead phrase this as "The expanded name doesn't provide more value to typical users than the acronym, the names becomes too long, and the acronym is unique in the field". I wonder if we could still include the
|
I like the shorter names. personally i'd abbreviate the followings too: StochasticGradientDescent -> SGD. GeneralizedAdditiveModel -> GAM Ordinary Least Squares Let's not abbreviate in the documentation, though cc @shmoradims @markusweimer do you think those are reasonable, compared to other frameworks? Just to summarize, main benefit of reverting back to acronyms is that we go from this:
to
|
@eerhardt even if we add the Trainer name to the classes the method names remain still names. |
Summary :
|
#2172 follow up on this one.
So right now we have mix of trainers names.
I want to standardize them.
We have following zoo of naming patterns:
LogisticRegression
SdcaMultiClassTrainer
SgdNonCalibratedBinaryTrainer
LinearSvmTrainer
LightGbmMulticlassTrainer
MulticlassLogisticRegression
My proposal is following:
{AlgoName}(optional){Calibrated/NonCalibrated}{TypeOfTask}Trainer
Where AlgoName is full name without abbreviations (SDCA->StochasticDualCoordinateAscent, Linearsvm ->LinearSupportVectorMachines) with exception of LightGBM.
I would also prefer to explicitly specify TypeOfTask even if algorithm exist only for one type. (Which would create weird abominations like OneVersusAllMulticlassTrainer, but i'm fine with that)
Does that sound good for you?
@sfilipi @TomFinley @eerhardt @yaeldekel
The text was updated successfully, but these errors were encountered: