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

PEP 696 input type default #28

Open
nstarman opened this issue Feb 8, 2023 · 4 comments
Open

PEP 696 input type default #28

nstarman opened this issue Feb 8, 2023 · 4 comments

Comments

@nstarman
Copy link
Contributor

nstarman commented Feb 8, 2023

Thinking about this more, we could even target PEP 696 and define:

ArrayT = TypeVar('ArrayT', bound=Array)
ArrayLikeT = TypeVar('ArrayLikeT', default=float | ArrayT)

class Cosmology(Generic[ArrayT, ArrayLikeT], Protocol): ...

Cosmologies using numpy arrays could then accept numpy.typing.ArrayLike inputs, and e.g. cosmo.comoving_distance([z1, z2]) would remain legal, which is a big plus in my mind.

Originally posted by @ntessore in #27 (comment)

@nstarman
Copy link
Contributor Author

nstarman commented Feb 9, 2023

I think the way users will expect is something like this:

ArrayT = OutputT = TypeVar('ArrayT', bound=Array)
InputT = TypeVar("InputT", default=float | ArrayT)

class Cosmology(Protocol[InputT, OutputT): ...

Unfortunately, I think this will always require both type inputs since InputT gets bound before OutputT. We can fix this with https://mypy.readthedocs.io/en/stable/extending_mypy.html by having Cosmology[OutputT] map to Cosmology[OutputT, OutputT]. But we might need a plugin for pyright as well... does pyright even support plugins?

@ntessore
Copy link
Contributor

ntessore commented Feb 9, 2023

I'm not sure I follow. What we need to prescribe in the first instance is the array type that the cosmology instance works with: Cosmology[ArrayT].

Then we have a situation where the input type can be array-like instead of being the exact array type. So we add a second type to the generic for that: Cosmology[ArrayT, ArrayLikeT].

Isn't that a good interface up to this point?

The point of the issue is then merely that we could go one step further and leverage the PEP 696 semantics to give a sensible default value to ArrayLikeT.

We could also not do that at all, because defining cosmology protocol implementations is really only something that vendor libraries do, not users in their day to day work.

@nstarman
Copy link
Contributor Author

nstarman commented Feb 9, 2023

My point is that the expectation is probably

Protocol[InputT, OutputT] not Protocol[OutputT, InputT].

But InputT with a default can't precede OutputT, without. At least not normally. We can make Protocol[InputT, OutputT] work as expected for mypy with https://mypy.readthedocs.io/en/stable/extending_mypy.html, but I'm not sure if that will work for pyright.

@ntessore
Copy link
Contributor

ntessore commented Feb 9, 2023

Yeah, I can see that if we phrased it in terms of input and output types, but I think in terms of array and array-like types it's fine.

In any case I think we can separate this into three issues and discussions

  • support float input
  • support array-like input
  • support PEP 696 for default array-like type

What I meant to say above is that I think it would even be fine to support array-like input without PEP 696 (or other) default behaviour, because it's something that needs to be specified once per cosmology implementation.

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

2 participants