-
Notifications
You must be signed in to change notification settings - Fork 2
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
Allow floats #27
Allow floats #27
Conversation
Signed-off-by: nstarman <[email protected]>
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #27 +/- ##
=======================================
Coverage 72.37% 72.37%
=======================================
Files 13 13
Lines 286 286
=======================================
Hits 207 207
Misses 79 79
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
I think this covers all the inputs. |
I wonder if it's worth adding an abstraction layer to our input type definition here. We could add a new ArrayLike = Union[float | ArrayT]
...
def scale_factor(self, z: ArrayLike[ArrayT], /) -> ArrayT:
... |
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 |
I agree it may be worth expanding the Generic type to be |
Yes, that's exactly the intention. The default behaviour is to have |
I see. Sorry for the confusion. Yes, this would be a good addition when that PEP is rolled out!
So that inputs to functions can be changed as you said, but output types remain Array. This can be supported by variadic generics so that only 1 or both types can be given. |
Fair enough. I am just a little wary of restricting functionality that currently is universally supported. |
We might be able to support it now by going for a variadic generic implementation. This requires a Mypy plugin, which might be quite simple. I'll take a look. |
This already works right now if you use from typing_extensions import TypeVar
ArrayT = TypeVar('ArrayT', bound=Array)
ArrayLikeT = TypeVar('ArrayLikeT', default=float | ArrayT)
class Cosmology(Generic[ArrayT, ArrayLikeT], Protocol): ...
reveal_type(Cosmology[Array]) # Cosmology[Array, float | Array] |
I did a quick experiment using the typing_extensions |
Ok cool. It works well with pyright so I assumed mypy would be the same. We can also leave this for now if it's blocking. |
Pyright has a faster release exhedule. I do think this is a blocker for just floats, which we do want to support. I'll make a reminder issue to return to this, because it's definitely a valuable addition. |
I added a short table to the docs on which array libraries allow the addition of an array and a float. |
The review got dismissed because I pushed to the branch. The branch protection rules are set up so that an approval only applies to the most recent version of the PR. |
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Needed to add a little bit of docs to get the toctree working. |
Signed-off-by: nstarman [email protected]
Description
Allow float inputs to redshift-dependent methods.
PR Checklist
CHANGES.rst
file. See existing changelog for examples.docs
folder.