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

explicitly check for Python scalars in constant tests #15

Closed
wants to merge 1 commit into from

Conversation

pmeier
Copy link
Contributor

@pmeier pmeier commented Jun 10, 2021

Fixes #14.

@rgommers
Copy link
Member

I don't think this is a good idea, see data-apis/array-api#154 (comment).

isinstance checks are in general very restrictive. It's hard to express how to be precise enough in text, but if it properly ducktypes at runtime, and statically types as the given type (float in this case), that should be considered spec-confirming.

The same applies to shape tuples, e.g. TensorFlow has a custom Shape object that ducktypes as a tuple.

@pmeier
Copy link
Contributor Author

pmeier commented Jun 10, 2021

data-apis/array-api#169 clarified

Each constant must have a Python floating-point data type (i.e., float) and be provided as a Python scalar value. [Emphasis mine]

So for the current spec duck typing is not allowed and the tests proposed by me should happen. We could switch the the check to numbers.Number to make it more permissive.

I'm fine with allowing other objects if there is a precise definition of what interfaces the object needs to provide to be considered compliant. IMO

if it properly ducktypes at runtime

is too vague for that.

@rgommers
Copy link
Member

Yes, we should fix that wording.

@asmeurer
Copy link
Member

This looks good, assuming we clarify whether it should be float or Number. You can ignore GitHub actions.

@BvB93
Copy link

BvB93 commented Jul 28, 2021

So for the current spec duck typing is not allowed and the tests proposed by me should happen. We could switch the the check to numbers.Number to make it more permissive.

Something to keep in mind is, in their current state, the number.Number subclasses are pretty much useless for static typing (python/mypy#3186).

@posita
Copy link

posita commented Jul 30, 2021

Not my fight, but I eventually settled on this approach for numerical duck-type static and runtime checking using Protocols. (I gave up trying to use the numerical tower.)

☝️ That approach is a little convoluted, but I'm happy to shed more light, if helpful or desired. In short, most of that file is setting the stage for its SupportsOutcome protocol. (An "outcome" is a dyce term, but think of it as describing only the parts of numbers.Real that the library cares about. In other words, SupportsOutcome attempts to describe number-like objects that can be passed to library APIs where "outcomes" are expected with a reasonable shot that things won't blow up.) In theory, SupportsOutcome could be used directly, even though dyce doesn't do that. The other things in that file basically comprise a desperate attempt to enable (semi-)performant runtime type checking around SupportsOutcome.

Again, I'm happy to talk shop if my technique proves useful outside my narrow corner of the Pythonverse.

@posita
Copy link

posita commented Dec 15, 2021

I don't know if you saw, but I pulled out a lot of my number typing work-arounds into their own package called numerary. If this journey lines up with your own, numerary could be an intermediary solution until python/mypy#3186 is fixed. (Alternatively, the techniques used therein might provide inspiration if you can't take a dependency.) Happy to consult here, if helpful.

@honno
Copy link
Member

honno commented Dec 16, 2021

I don't know if you saw, but I pulled out a lot of my number typing work-arounds into their own package called numerary. If this journey lines up with your own, numerary could be an intermediary solution until python/mypy#3186 is fixed. (Alternatively, the techniques used therein might provide inspiration if you can't take a dependency.) Happy to consult here, if helpful.

@posita I saw this before, awesome project! I'll at some point explore using it in a branch, and from there we can see if 1) it really can cover all the bases 2) we're happy to add the dependency.

@posita
Copy link

posita commented Dec 16, 2021

… I'll at some point explore using it in a branch, and from there we can see if … it really can cover all the bases … .

If you have any questions or encounter any friction or gaps, please don't hesitate to reach out. I'm eager to make sure this is useful and accessible to others, and happy to help out where I can.

@honno
Copy link
Member

honno commented Dec 21, 2021

Closed in favour of #52, as some of the array module usage was unnecessary after data-apis/array-api#169 (and so added unwanted dependencies).

Whilst the docs say constants are "Python floats i.e. float", we're just checking they instance check with SupportsFloat, as that allows duck typed constants which some adopters might be using. Once we get more array libraries actually adopting we can see in practice how constants work out i.e. we may be too lenient, or we may even be too strict.

Thanks @pmeier for this PR, as the introduced nan and inf test logic was useful.

(@posita so yeah for this we probably don't want numerary for these tests, but there's lots of instances in the test suite here where we might do well to use it. Even just performance things, and generally getting rid of numerical towers would be nice.)

@honno honno closed this Dec 21, 2021
@pmeier pmeier deleted the constants-python-scalars branch December 21, 2021 10:56
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.

Test explicitly that constants are Python scalars
6 participants