dgasmith / opt_einsum

⚡️Optimizing einsum functions in NumPy, Tensorflow, Dask, and more with contraction order optimization.
https://dgasmith.github.io/opt_einsum/
MIT License
863 stars 68 forks source link

[BUG FIX] Add shapes support to parse_einsum_input #200

Closed janeyx99 closed 2 years ago

janeyx99 commented 2 years ago

Description

This PR adds support for the shapes kwarg for parse_einsum_input. This also fixes a bug where contract_path takes in the shapes parameter but parse_einsum_input is called without respecting the shapes parameter.

The bug results because even though the user had input shapes as True, contract_path will call parse_einsum_input before caring about shapes, and within parse_einsum_input, will call .shape on an array that is already a shape.

Todos

Notable points that this PR has either accomplished or will accomplish.

Questions

Status

codecov[bot] commented 2 years ago

Codecov Report

Merging #200 (f19e8eb) into master (3824e4f) will decrease coverage by 0.01%. The diff coverage is 100.00%.

janeyx99 commented 2 years ago

Welp, lol, messing with the parameters did not go well--I'll take another look tomorrow but let me know if you have any advice--I kept getting errors when I didn't use *operands about shapes getting multiple inputs :c.

dgasmith commented 2 years ago

Welp, lol, messing with the parameters did not go well--I'll take another look tomorrow but let me know if you have any advice--I kept getting errors when I didn't use *operands about shapes getting multiple inputs :c.

That is very odd- can you post the exact error message?

janeyx99 commented 2 years ago

That is very odd- can you post the exact error message?

We are good now!

janeyx99 commented 2 years ago

Is there anything on my end I should do to merge this PR?

dgasmith commented 2 years ago

I'll recap from a comment above:

We have a faulty test in this case, It just so happens that len(arr) provides the shape of the first dimension. We need to ensure that a corresponding test fails for the given test case. A fairly minor point, but the tests as written can be misleading in the future.

I think we need to make sure the test properly raises if an array is passed as a shape. Either that or detect automatically if the object has a .shape attribute or not.

janeyx99 commented 2 years ago

Ah oops, I didn't see that last comment. I've added a test case now.

janeyx99 commented 2 years ago

Lint failure looks unrelated

dgasmith commented 2 years ago

Looks like MyPy bumped, would you mind fixing? Removing the "type: ignore" from line opt_einsum/paths.py:1251 should patch it.

dgasmith commented 2 years ago

Amazing, thank you!