Skip to content

Ignore whitespace in gufunc signature#8267

Merged
jrbourbeau merged 2 commits intodask:mainfrom
jrbourbeau:gufunc-whitespace-2
Oct 18, 2021
Merged

Ignore whitespace in gufunc signature#8267
jrbourbeau merged 2 commits intodask:mainfrom
jrbourbeau:gufunc-whitespace-2

Conversation

@jrbourbeau
Copy link
Copy Markdown
Member

This is a follow-up to #8049. As pointed out by @GenevieveBuckley here #7972 (comment), the original example in #7972 is still failing on the current main branch. This is because there are two places we pass a signature to np.vectorize and #8049 only took care of one of them. This PR ensures that both are handled properly.

cc @GenevieveBuckley

Closes #7972

@github-actions github-actions bot added the array label Oct 14, 2021
Copy link
Copy Markdown
Contributor

@GenevieveBuckley GenevieveBuckley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm, this completely fixes #7972 🎉

I've left a small suggestion to include a bit more detail in the comment belonging to the test, but otherwise it is good to go.

def gufoo(x):
return np.linalg.inv(x)

gufoo(a) # Previously this would raise an error
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a bit more context in this comment. Previously what? And why?

The comment you have a bit further up in this PR is great, it makes it very clear what is happening and why. Perhaps we can use bits of that here too. The bit about failing only for numpy versions before numpy/numpy#19627 is especially important.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed an update to the comments in this test -- let me know what you think

@GenevieveBuckley
Copy link
Copy Markdown
Contributor

This is ready to merge @dask/maintenance

@jrbourbeau
Copy link
Copy Markdown
Member Author

Thanks for reviewing @GenevieveBuckley

@jrbourbeau jrbourbeau merged commit ee6814f into dask:main Oct 18, 2021
@jrbourbeau jrbourbeau deleted the gufunc-whitespace-2 branch October 18, 2021 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dask Gufunc signatures should ignore whitespace

2 participants