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

BUG: fix nout=2 ufuncs #109

Merged
merged 2 commits into from
Apr 7, 2023
Merged

BUG: fix nout=2 ufuncs #109

merged 2 commits into from
Apr 7, 2023

Conversation

ev-br
Copy link
Collaborator

@ev-br ev-br commented Apr 6, 2023

A minimal fix for a bug in handling of out=(ndarray, ndarray).

@ev-br ev-br marked this pull request as draft April 6, 2023 10:51
Base automatically changed from ndarray_annot to main April 6, 2023 11:08
@ev-br ev-br changed the title WIP: fix I/O handling of nout=2 ufuncs BUG: fix nout=2 ufuncs Apr 6, 2023
@ev-br ev-br marked this pull request as ready for review April 6, 2023 13:58
Comment on lines 132 to 137
out1, out2 = out
if out1.shape != out2.shape or out1.dtype != out2.dtype:
raise ValueError("out1, out2 must be compatible")
if num_outs == 2:
if out1 is not None or out2 is not None:
raise TypeError(
"cannot specify 'out' as both a positional and keyword argument"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't thin this logic is correct. If num_outs == 2 and out = (None, None), here we are setting setting out1 and out2 to None.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, thanks for spotting this! Fixed in a855a37. The relevant test needs to stay xfailed for a while, since we have hardcoded the name "out" for postprocessing.

The current state is that out=tuple works, and there is an xfailed test.

Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

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

Nice. Note that before, since we were not using normalize, we were not casting x1 and x2 here. I guess that that happened to work because this function is a composite of other functions, but yeah...

@ev-br ev-br marked this pull request as draft April 7, 2023 08:57
@ev-br ev-br marked this pull request as ready for review April 7, 2023 08:59
@ev-br
Copy link
Collaborator Author

ev-br commented Apr 7, 2023

Let's get this in then --- am adding out1=, out2= to the list of current deficiencies, gh-73

Indeed, not calling normalize was when composing normalize-decorated functions. Which is indeed brittle and hopefully not happening anymore with the final version of the wrapper/impl split.

@ev-br ev-br merged commit 083a0e6 into main Apr 7, 2023
@ev-br ev-br deleted the nout=2 branch April 7, 2023 09:04
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.

2 participants