I don't know if it's the correct fix because these functions have a lot of logic going on that I have never looked at before (and haven't looked at too closely). Feel free to close if the right solution is a bit more involved.

mhvk added 00 - Bug component: numpy._core 09 - Backport-CandidatePRs tagged should be backported labels

Jan 10, 2018

mhvk added this to the1.15.0 release milestone

Jan 10, 2018

Copy link

Contributor

mhvk commentedJan 10, 2018

@larsoner - thanks for the PR. It took me along time to see what the heck is going on in the parsing, but now I understand that, basically, theoptimize path did not include broadcasting -- I must add it surprised me that broadcasting was even allowed on identical indices (rather than just on the..., but that seems to be the case before too).

Anyway, I think this PR is correct.@dgasmith - could you have a look?

This was referencedJan 10, 2018

Major slowdown as einsum optimize path used even for two arguments#10357

Closed

einsum broadcast regression (with optimize=True)#10343

Closed

Copy link

Contributor

dgasmith commentedJan 10, 2018

I need to think on it a bit, that parsing tech is fairly complex as folks want quite the variety of input possibilities. I would consider adding more tests that cover the "double broadcasting" cases where 1-dims are mixed with ellipses like in the original issue.

Copy link

Contributor

dgasmith commentedJan 13, 2018

This might hit issues in the BLAS calls. Please try the following two cases:

a=np.random.rand(1,5)b=np.random.rand(5,5)np.einsum("...ij,...jk->...ik",a,a)np.einsum("...ij,...jk->...ik",a,b)

I suspect we may have to go in and change the BLAS logic to bounce broadcasting cases as well.

FIX: Fix einsum optimize logic for singleton dimensions

5e8301c

larsoner force-pushed thefix-einsum branch from4b08686 todf0a43cCompare

January 14, 2018 06:53

Copy link

ContributorAuthor

larsoner commentedJan 14, 2018

Rebased due to conflict and test added. I'm not on my dev machine now so I can't easily run the new tests so I'll let the CIs do the work :)

ENH: Add broadcasting test

d5622a4

larsoner force-pushed thefix-einsum branch fromdf0a43c tod5622a4Compare

January 14, 2018 22:45

Copy link

ContributorAuthor

larsoner commentedJan 15, 2018

@dgasmith @mhvk tests are indeed a bit broken now. It might take me a while to fix this, so feel free to take over, or I can find time in the coming weeks to work on it.

Copy link

Contributor

dgasmith commentedJan 15, 2018

Yup, ill need to go in and really take a look at whats going on. Ill get to it in the next few days.

dgasmithand others added2 commits

January 17, 2018 19:19

Patches up broadcasting einsum issues for BLAS cases

10480ad

FIX: Deduplicate code

6348345

larsoner changed the title~~FIX: Fix einsum optimize logic for singleton dimensions~~MRG: Fix einsum optimize logic for singleton dimensions

Jan 18, 2018

Copy link

ContributorAuthor

larsoner commentedJan 18, 2018

Okay@dgasmith I deduplicated the code a bit, feel free to look to see if you're happy.

CIs are happy, ready for review/merge from my end.

dgasmith approved these changes

Jan 18, 2018

View reviewed changes

Copy link

Contributor

dgasmith left a comment

Choose a reason for hiding this comment

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

Looks good to me, the NumPy maintainers might have a few convention thoughts.

Copy link

Member

eric-wieser commentedJan 18, 2018

Can you loop me in on what theMRG prefix means? I've seen it used a bunch in matplotlib too

eric-wieser reviewed

Jan 18, 2018

View reviewed changes

numpy/core/einsumfunc.py

		left_dims= {dim:sizefordim,sizein
		zip(input_left,tmp_operands[0].shape)}
		right_dims= {dim:sizefordim,sizein
		zip(input_right,tmp_operands[1].shape)}

Copy link

Member

eric-wieserJan 18, 2018

Choose a reason for hiding this comment

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

Why not useidx_rm here? The other entries in this dict are being ignored

Copy link

ContributorAuthor

larsonerJan 18, 2018

Choose a reason for hiding this comment

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

Hereinput_left is e.g.'ik', so this seems like a straightforward / quick way to get the indices to line up. Are you thinking something like this should be done instead?

if any(tmp_operands[0].shape[input_left.index(ind)] != tmp_operands[1].shape[input_right.index(ind)] for ind in idx_rm)

Copy link

Member

eric-wieserJan 18, 2018•
edited
Loading

Choose a reason for hiding this comment

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

You're right, what you had was better and probably faster too

Copy link

ContributorAuthor

larsoner commentedJan 18, 2018

Can you loop me in on what the MRG prefix means? I've seen it used a bunch in matplotlib too

It's a way for me (contributor) to signal to you (reviewers) that the PR is ready for review / merge from my perspective. In other projects I work on, maintainers historically have then changed the title as they approve to e.g.MRG+1,MRG+2 etc., though GitHub recently came out with the "approve" / "request changes" stuff so now that mechanism gets used more than additional title changes from the reviewer end.