sktime/sktime

[ENH] managing defaults in public and private interfaces

Open

#4 809 ouverte le 2 juil. 2023

Voir sur GitHub
 (3 commentaires) (1 réaction) (0 assignés)Python (1 192 forks)batch import
API designenhancementgood first issuemodule:base-framework

Métriques du dépôt

Stars
 (7 162 stars)
Métriques de merge PR
 (Merge moyen 26j 10h) (86 PRs mergées en 30 j)

Description

update @fkiraly - from discussion below and in linked issues/PR (#4810, #4780), it seems that the consensus is to remove defaults in private methods.

This is a good first issue - a contributor can pick any of the template slot private methods (_transform, _update, etc - look in extension templates for more) and remove the defaults throughout the code base (please post which to avoid duplication).

Note that this may throw up some errors, in which case the draft PR will be useful and informative in helping with bugfixing.


From this discussion https://github.com/sktime/sktime/pull/4780#discussion_r1246621774, FYI @benHeid, @yarnabrina.

Context

sktime has a public/private design using the template pattern, e.g., fit / _fit, where fit calls _fit.

Defaults are currently present in the public (e.g., fit) and the private (e.g., _fit) method - and it turns out the defaults in the private methods are inconsistent. Whereas there is only one single default for the public method, and one location for those (the base class).

However, the private methods should not be directly called and usually get all parameters passed, so there is in-principle no need for any defaults in the private methods (and no harm caused from the private defaults being inconsistent, since they are always overridden).

Question

The question is how to deal with the defaults in the private methods, and the current inconsistency of those defaults.

Some options on this I can see:

  1. end state: ensure the private defaults are always consistent with the public defaults; action: change the current inconsistency to that end state.
  2. end state: private methods never have defaults (only assumes); action: remove any current defaults from private methods.

FK opinion

My personal opinion is that 2 is better than 1, since 2 is better maintainable.

Suppose a default changes somewhere, then this requires a change only in one place (the base class), not in 100s of classes including classes that may live in third party packages (as would be required in option 1).

The important argument imo is that option 1 would require maintenance action by all extending users/developers, while option 2 would limit the maintenance scope to a single line of code (in the base class). With the added oddity that in option 1, the maintenace actions in private methods have no effect on the logic at all.

Another argument in favour of 2 is that it would immediately surface any failures to pass arguments that should be passed to the private methods - as an exception would be raised. We had a couple cases where this was the source of errors, e.g., https://github.com/sktime/sktime/issues/4434

Guide contributeur