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

Avoid typechecking scalar arguments in transform.xy/rowcol #2284

Open
wants to merge 3 commits into
base: maint-1.2
Choose a base branch
from

Conversation

groutr
Copy link
Contributor

@groutr groutr commented Sep 8, 2021

Resolves #2283

@sgillies
Copy link
Member

sgillies commented Sep 8, 2021

@groutr I tried a different approach in #2285, checking for __iter__ is the most general test and this aspect of being a collection is the only thing we require (we need the inputs to be zip-pable and that requires __iter__).

@groutr
Copy link
Contributor Author

groutr commented Sep 8, 2021

@sgillies Checking isinstance(x, Iterable) is equivalent to checking for __iter__ (https://docs.python.org/3/library/collections.abc.html#collections.abc.Iterable)

In this PR, if scalar arguments are passed, I wrap them in an iterable container before applying the transformation. That makes the inputs zip-able. After the transform, I return based on if the inputs were scalar or not.

Another important part of this PR is the chained exception, preserving the TypeError that is raised from Affine. That TypeError provides important type information if the multiply failed.

@sgillies
Copy link
Member

sgillies commented Sep 8, 2021

@groutr I appreciate your work and don't dismiss this PR lightly! I only want to add the minimum of new code and behavior and am not entirely sure that everything we want to support subclasses Iterable.

raise from would be appropriate for 1.3.0 for sure and since we're being more selective about numpy versions there I think testing Iterable would be fine too.

@sgillies sgillies added this to the post-1.3 milestone Apr 11, 2022
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.

None yet

2 participants