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

Add future warnings about breaking changes in 3.0.0 #765

Merged
merged 7 commits into from
Feb 16, 2024

Conversation

MatthiasSchmidtblaicherQC
Copy link
Contributor

The warnings reflect the current state of #764 and #677, should be updated accordingly if we make changes.

Checklist

  • Added a CHANGELOG.rst entry

@MatthiasSchmidtblaicherQC MatthiasSchmidtblaicherQC changed the title add future warnings on breaking changes in 3.0.0 Add future warnings about breaking changes in 3.0.0 Feb 15, 2024
@stanmart
Copy link
Collaborator

stanmart commented Feb 15, 2024

Sounds good! What about only showing the warning, if the user tries to invoke the function in a 3.0.0-incompatible way, though? I'm thinking of something like:

    @_positional_args_warning(unchanged_args=("X", "sample_weight", "offset"))
    def predict(
        self,
        X: ShapedArrayLike,
        sample_weight: Optional[ArrayLike] = None,
        offset: Optional[ArrayLike] = None,
        alpha_index: Optional[Union[int, Sequence[int]]] = None,
        alpha: Optional[Union[float, Sequence[float]]] = None,
    ):
    ...

where the decorator looks something like this:

def _positional_args_warning(unchanged_args=(), unchanged_args_number=None):
    """Raise a FutureWarning if more than `unchanged_args_number` positional arguments are passed."""
    if unchanged_args_number is None:
        unchanged_args_number = len(unchanged_args)

    def decorator(func):
        @wraps(func)
        def wrapper(*args, **kwargs):
            if len(args) > unchanged_args_number + 1:  # +1 for self
                excluded_string = (
                    f"Arguments other than {', '.join(unchanged_args)}"
                    if unchanged_args
                    else "All arguments"
                )
                name = func.__name__ if func.__name__ != "__init__" else args[0].__class__.__name__

            warnings.warn(
                    f"{excluded_string} to `{name}` will become keyword-only in 3.0.0.",
                    FutureWarning
            )
            return func(*args, **kwargs)

        return wrapper

    return decorator

The second, optional parameter for the decorator is useful when we change the order of the arguments:

    @_positional_args_warning(("X", "y", "sample_weight", "offset"), 2)
    def covariance_matrix(
        self,
        X=None,
        y=None,
        mu=None,
        offset=None,
        sample_weight=None,
        dispersion=None,
        robust=None,
        clusters: Optional[np.ndarray] = None,
        expected_information=None,
        store_covariance_matrix=False,
        skip_checks=False,
    ):
    ...

@MatthiasSchmidtblaicherQC
Copy link
Contributor Author

I would be fine with being overabundant and slightly annoying with the FutureWarnings, at least they alert the user that there is a major release coming up (and users can always filter them). Your more targeted approach seems of course superior though and, since you already built it, I think that we should go for it. People who actually want to work with v2.7.0 will thank you. Would you have the time adjust the PR accordingly?

@stanmart
Copy link
Collaborator

Sure, I can do it tomorrow. It's just a quick proof of concept now but I think this approach should work without many issues.

But I'm also okay with the current more cautious warnings. Maybe that will make more people switch to v3. :) Let me know which one we should go for.

@MatthiasSchmidtblaicherQC
Copy link
Contributor Author

MatthiasSchmidtblaicherQC commented Feb 15, 2024

Since we are not sure, it is probably best to just follow what other packages do. pd.concat had a similar breaking change in 2.0.0, which they warned about using... a decorator which looks quite similar to what you suggested :).

So I guess we should go with the decorator approach that only warns if a behavior that would break in the future is detected. We could just import the decorator from pandas, but, if I see correctly, that one does not take into account reshuffling of arguments as in the covariance_matrix case.

@stanmart
Copy link
Collaborator

stanmart commented Feb 16, 2024

Oops, I might have reinvented the wheel... :) I made the changes. I think it works, but could you please also check if it does what it's supposed to do?

@MatthiasSchmidtblaicherQC
Copy link
Contributor Author

Thanks a lot! I tested and it works as expected for me.

@MatthiasSchmidtblaicherQC MatthiasSchmidtblaicherQC merged commit 96e6534 into main Feb 16, 2024
21 checks passed
@MatthiasSchmidtblaicherQC MatthiasSchmidtblaicherQC deleted the future-warnings branch February 16, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants