Skip to content
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

[API] fit, predict and other functions should accept X,y,smaple_weight as first parameters by default #88

Open
rflamary opened this issue Feb 14, 2024 · 2 comments
Labels
domain-aware api enhancement New feature or request good first issue Good for newcomers

Comments

@rflamary
Copy link
Collaborator

In order to stay compatible with skleranAPI we want all estimatoer (and Adapters) to have functions that accept unnamed sample_weightby defalult.

For the moment we have for Adapter and DAEstimator:

    @abstractmethod
    def fit(self, X, y=None, sample_domain=None, *, sample_weight=None):
        """Fit adaptation parameters"""

I suggest we change it to

    @abstractmethod
    def fit(self, X, y=None, sample_weight=None sample_domain=None, *):
        """Fit adaptation parameters"""

that will allow to do things like

   model.fit( X, y, sample_weight, sample_domain):

the named sample_domain will still be necessary for pipelines

@rflamary rflamary added enhancement New feature or request good first issue Good for newcomers domain-aware api labels Feb 14, 2024
@rflamary rflamary changed the title [API] fit, predict and otehr fnction should acept X,y,smaple_weight as first parameters by default [API] fit, predict and other functions should accept X,y,smaple_weight as first parameters by default Feb 14, 2024
@rtavenar
Copy link

Hi,

I'd like to start contributing to skada and it seems this issue could be a good start. Is there a consensus on @rflamary 's suggestion? If yes, is it OK if I start a PR on this?

@kachayev
Copy link
Collaborator

kachayev commented Feb 15, 2024

Let's make 'sample_domain' parameter named-only (as it previously was). I know recall that the change was made to have ability to call

fit(X, y, sample_domain)

(no names, just ordering). If we put 'sample_weight' third, the ability to do

fit(X, y, None, sample_domain)

would be frustrating. To avoid this, we just need to declare

    @abstractmethod
    def fit(self, X, y=None, sample_weight=None, *, sample_domain=None, **params):
        """Fit adaptation parameters"""

This will force 'sample_domain' to be always named, removing the risk of putting it instead of weights (technically, we won't be able to spot the problem as we allow 'sample_domain' to be given as None).

WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain-aware api enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants