Confusing UX with default values about pytorch-onfire HOT 3 OPEN

On the one hand, I think it is a nice feature to provide reasonable default values for each field type, for the following reasons:

• When using the library, it is a common scenario to instantiate these classes many times (once for each feature), requiring to specifying these parameters several times (and probably with the same values). Providing default values avoids that need, making the code less verbose and more clear.
• It makes the library very friendly to beginner users, who won't need to worry about many settings.

On the other hand, I fully agree with you that the UX is confusing and can be improved.
Probably, one of the reasons of such confusing behavior is the overuse of a single parameter to do many things. What if it uses two separate parameters instead? Following your example with "scaler"...

• scaler: the scaler strategy to be used.
  • 'standard_scaler' (default): uses StandardScaler with default arguments.
  • 'custom': uses the scaler transform provided in custom_scaler parameter.
  • None (False, 0): uses no scaler.
• custom_scaler: when scaler=='custom', this parameter specify the scaler transform to be used.

It is a similar to the pattern followed by sklearn, for instance in SimpleImputer (strategy/fill_value parameters)

Does it make sense to you?

from pytorch-onfire.

I like your idea, enables the simplicity of use together with the flexibility for power users.
And I really like that matches the pattern of some pieces of sklearn.

But... it feels a bit of too-much-paperwork filling 2 params for single purpose.
Re-reading my initial comment I realize that what it hurts the most, is the issue with None interpreted as "standard_scaler".

Maybe an option in the middle is a good choice as well.

Signature:

class ContinuousFeature(BaseFeature):
    def __init__(self, key=None, preprocessor=None, imputer=None, scaler='standard_scaler', log=False,
                 log_auto_scale=True):
    ...

and then:

• scaler:
  • 'standard_scaler' (default): uses StandardScaler with default arguments.
  • None (False, 0): uses no scaler.
  • <custom-scaler-object>: user defined scaler transformer to be used.

from pytorch-onfire.

I'm sorry for reviving this issue... but we have been using this and I thought that maybe it would make sense to do it like this, from a user perspective:

Thus, it would be something like:

class ContinuousFeature(BaseFeature):
def init(self, key=None, preprocessor=None, imputer=None, scaler=StandardScaler(), log=False,
log_auto_scale=True):
...

and then:

• default is StandardScaler()
• None would be no scaler
• Pass other scaler object, uses the custom scaler.

and, therefore:

• self.scaler = scaler

which is much more readable, in my opinion.

from pytorch-onfire.