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

Strange overload signature with v0.12.1 #374

Open
mhostetter opened this issue Jul 26, 2024 · 11 comments
Open

Strange overload signature with v0.12.1 #374

mhostetter opened this issue Jul 26, 2024 · 11 comments

Comments

@mhostetter
Copy link
Contributor

Hey guys! I saw there was a flurry of activity to get to v0.12. Thanks for that! I just upgraded to it, but noticed a strange artifact in an overloaded signature in one of my projects.

I have this signature:

@overload
def time_domain(
    x: npt.NDArray,  # TODO: Change to npt.ArrayLike once Sphinx has better overload support
    *,
    sample_rate: float | None = None,
    centered: bool = False,
    offset: float = 0.0,
    ax: plt.Axes | None = None,
    diff: Literal["color", "line"] = "color",
    **kwargs,
): ...


@overload
def time_domain(
    t: npt.NDArray,  # TODO: Change to npt.ArrayLike once Sphinx has better overload support
    x: npt.NDArray,  # TODO: Change to npt.ArrayLike once Sphinx has better overload support
    *,
    sample_rate: float | None = None,
    centered: bool = False,
    offset: float = 0.0,
    ax: plt.Axes | None = None,
    diff: Literal["color", "line"] = "color",
    **kwargs,
): ...


def time_domain(  # noqa: D417
    *args,
    sample_rate: float | None = None,
    centered: bool = False,
    offset: float = 0.0,
    ax: plt.Axes | None = None,
    diff: Literal["color", "line"] = "color",
    **kwargs,
):

It was rendered like this in v0.11.14 with Sphinx v7.3.7, which I'm pretty happy with. https://mhostetter.github.io/sdr/v0.0.22/api/sdr.plot.time_domain/

image

But in v0.12.1 and Sphinx v7.3.7, it renders like this. https://mhostetter.github.io/sdr/v0.0.x/api/sdr.plot.time_domain/

image

Have any idea what might be causing that?

@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 26, 2024

Seems like our new support for type parameters (supported in Sphinx v7.3+) has taken an affect. It seems like npt.NDArray is a generic type with a type parameter set to _Scalar_type_co.

I think Sphinx's new ability to show metadata on an Annotated type (introduced in Sphinx v7.4) is also taking affect here, but it doesn't look like ndarray inherits from the Annotated class. The C source for makes it difficult to determine this.

@jbms
Copy link
Owner

jbms commented Jul 27, 2024

I take it the problem is the poor line wrapping?

Our wrap_signatures_with_css option currently also wraps the type parameters if it determines that wrapping is needed. It could be updated but in general it seemed like a losing game to try to reimplement proper wrapping.

Instead I added the option to format using black. Unfortunately black also has an outstanding bug with type parameters but hopefully that will be fixed soon. psf/black#4071

@mhostetter
Copy link
Contributor Author

I take it the problem is the poor line wrapping?

My concern was why the [_ScalarType_co] was showing up at all. I didn't know about Python's new type parameters. But since my signature doesn't use type parameters, would it make sense to suppress whatever recognized that type parameter, such that it is no longer documented?

@jbms
Copy link
Owner

jbms commented Jul 28, 2024

Currently type parameters are detected automatically even if you don't use the new python 3.12 syntax. We could add an option to disable that, I suppose. Is that preferable to fixing the formatting?

@mhostetter
Copy link
Contributor Author

Should the type parameters appear at all in my case? There's no output to the function. And it's not parameterized by any types. It just takes a npt.NDArray as an input.

A global disable switch would prevent this signature from (in my opinion) erroneously showing the type parameter, but the downside is all intentional type parameters would also be disabled. I wonder if my example is a bug in how the type parameters are/should be displayed.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 28, 2024

I think that since the type parameter in this case starts with a _, it probably should be disregarded.

I think it's worth automatically hiding the type parameter for specialized types, when a generic type's parameter is assigned a known type. It might be harder to detect what a known type is though. Not all libraries are compatible with sphinx' introspection.

@jbms
Copy link
Owner

jbms commented Jul 28, 2024

Should the type parameters appear at all in my case? There's no output to the function. And it's not parameterized by any types. It just takes a npt.NDArray as an input.

A global disable switch would prevent this signature from (in my opinion) erroneously showing the type parameter, but the downside is all intentional type parameters would also be disabled. I wonder if my example is a bug in how the type parameters are/should be displayed.

I see. It gets detected as having type parameters because the signature displays with type parameters even though you didn't specify any explicitly. That is presumably due to a limitation in generic type aliases prior to python 3.12. In fact I'm surprised it displays as NDArray at all rather than what it evaluates to.

I think in this case you could just specify the type as numpy.ndarray and that would fix the problem.

As far as making ArrayLike display better could try doing something like this:

https://github.com/google/neuroglancer/blob/3efc90465e702453916d2b03d472c16378848132/docs/conf.py#L202

@2bndy5

This comment was marked as resolved.

@jbms

This comment was marked as resolved.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 28, 2024

You're rather unapologetic when it comes to monkey patching python's runtime. I've never seen it wielded so freely in any other python projects.

@jbms
Copy link
Owner

jbms commented Jul 29, 2024

Monkey patching has a lot of downsides but sometimes it is the most practical option, especially when sphinx is involved.

For these numpy types I considered adding some logic in our type annotations transform code but just monkey patching numpy to ensure the aliases stayed unexpanded seemed much simpler. Since it only applies when building the documentation it doesn't even matter if it breaks things as long as those things aren't needed when building the documentation.

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

No branches or pull requests

3 participants