Skip to content

Overload the function signature of axline #29235

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

li-ruihao
Copy link

@li-ruihao li-ruihao commented Dec 5, 2024

PR summary

Originally within lib/matplotlib/axes/_axes.pyi,

There was a TODO item:

# TODO: Could separate the xy2 and slope signatures
    def axline(
        self,
        xy1: tuple[float, float],
        xy2: tuple[float, float] | None = ...,
        *,
        slope: float | None = ...,
        **kwargs
    ) -> AxLine: ...

The change in this PR is to overload the definition of axline with two separate definitions, with one needed to specify xy2 and the other to specify slope.

This PR closes issue #29234.

PR checklist

…pecify xy2, and another having the option to specify slope
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@li-ruihao li-ruihao changed the title Overloading the definition of axline Overload the function signature of axline Dec 5, 2024
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the | None options should go on the overload as the parameters xy2 and slope are not optional for the specific cases.

@ksunden
Copy link
Member

ksunden commented Dec 6, 2024

I have not thought this through completely yet, but I do think that these are made slightly harder by a few factors. I think Tim is right about removing the | None when those things are non-optional.

I am, however, not sure that that will have the desired result, especially once it goes through boilerplate for the purposes of pyplot.py. Currently boilerplate does not fully contemplate overloads for the functions it generates. A quick rereading of boilerplate I believe we only check the first overload (and even if we let one particular loop continue instead of early returning, not sure it would to the right thing). It could (and maybe should) do so, but that is a larger and more complicated change. In particular, I think if you remove the | None, which is the correct signature in each individual case, the logic currently used by boilerplate may make the pyplot version break by not including the None in the type hint (but I think it will still set the default...)

One option would be to move this method out of the autogenerated portion of pyplot and just include the overloads (and subsequent implementation) in the handwritten portion. We do this for a few things already. This is perhaps a reasonable short term action (though I would have to look again at the precise actions that would need to be taken there).

One thing that complicates the analysis at least it is the inclusion of **kwargs, which means that the signatures overlap more than you want to think of them logically, which could cause additional problems for type checkers (though I think this is a minor concern)

@li-ruihao
Copy link
Author

li-ruihao commented Dec 6, 2024

I initially removed the | None options, as you can see from the first commit to this PR.

However, in order to keep the original behaviors of the function without introducing any regression errors, I added the | None options back. When I tested my changes with the following test cases:

  1. using axline with xy2 as a parameter
  2. using axline with slope as a parameter
  3. using axline with both xy2 and slope as parameters
  4. using axline without xy2 or slope in the parameters

Test cases 1 and 2 worked as expected and successfully drew a line, test cases 3 and 4 threw errors with a message that says only one of xy2 or slope must be presented, which were working as expected as well. Therefore confirming the current changes did not introduce any regression errors.

It does make sense to remove the | None options, but it might involve more changes as the current implementation of axline specifies the default values for xy2 and slope to be None as well. Additionally, I am not sure if I should refactor any code because of overloading the function signature (i.e the actual function implementation)

@rcomer rcomer linked an issue Dec 12, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

TODO spotted: Separating xy2 and slope
3 participants