updates to 'dp' optimizer #119

jcmgray · 2019-12-02T14:59:46Z

Description

These updates the 'dp' optimizer in a few ways:

fixes some edge case bugs (Getting "not enough values to unpack" with optimize=dp #116, optimize='dp' gets confused by singleton factors #118) @yaroslavvb
allows it to be customized in a few ways (without any impact on performance in the default case):
- whether to target minimum flop count or minimize size
- whether to search through outer products
- how to set the cost_cap iterate strategy (on, off or manual)
a few tweaks to make the overhead lower for small contractions particularly
renames DynamicProgrammingOptimizer->DynamicProgramming and add it to the top namespace

I think with these, it would make sense to make the 'optimal' algorithm point at 'dp' since with the outer product search it should now find all the same contractions, and with the cost_cap turned off it's as fast for small contractions as well. This would fix #99. Thoughts @dgasmith? Also cc @mrader1248.

Todos

document customizable DynamicProgramming
one could identify now 'standard tensor network' contractions (i.e. every index appears exactly twice) at the beginning from the index count, and possibly switch to a faster method for computing resulting indices (simply the symmetric difference of inputs)
use a version of 'dp' for 'optimal'?
have an 'auto'value for the cost_cap that simply turns it off til the contraction is quite big

Status

Ready to go

codecov · 2019-12-02T15:05:46Z

Codecov Report

Merging #119 into master will increase coverage by 0.05%.
The diff coverage is 100%.

dgasmith

A few quick comments, I need to think a bit about the logic here.

dgasmith · 2019-12-02T15:18:41Z

opt_einsum/paths.py


                                # only if s1 and s2 are disjoint
-                                if s1 & s2 == 0:
+                                if not s1 & s2:


Can we collapse this and the below if to a single line to help with the indention?

Ah yes nice.

dgasmith · 2019-12-02T15:19:03Z

opt_einsum/tests/test_paths.py

@@ -197,6 +197,59 @@ def test_greedy_edge_cases():
    assert check_path(path, [(0, 1), (0, 2), (0, 1)])


+def test_dp_edge_cases_dimension_1():


Awesome, thanks for the extra tests!

Good idea to add these tests.

dgasmith · 2019-12-02T15:19:52Z

opt_einsum/paths.py

+                i_r = set()
+
+            # contraction indices:
+            i_cntrct = i1_cut_i2_wo_output - i_r


Can we write out cntrct (is it contract?), will help with readability.

I think it is (I didn't write these variable names originally), but yes can update.

Yes, it stands for contract. Don't know why I named it that way.. Maybe some line length issue..

dgasmith

Overall LGTM with a few more docs.

@mrader1248 Do you have time to comment here?

dgasmith · 2019-12-03T17:54:00Z

docs/source/dp_path.rst

+    import opt_einsum as oe
+
+    optimizer = oe.DynamicProgramming(
+        minimize='size',    # optimize for size (rather than FLOPs)


You may want to define size here.

dgasmith · 2019-12-03T17:56:51Z

opt_einsum/paths.py


-    >>> list(_bitmapset_indices(0b1001011))
-    [0, 1, 3, 6]
+def _dp_calc_legs(g, all_tensors, s, inputs, i1_cut_i2_wo_output, i1_union_i2):


Can we add short doc strings for these two functions.

mrader1248 · 2019-12-03T20:18:44Z

opt_einsum/paths.py

+            else:
+                # if the input has any index reductions, add single contraction
+                inputs.append(i_reduced)
+                inputs_contractions.append((j,) if i_reduced != i else j)


I see that my original code did not work if there was nothing more to optimise at this point. However, I find this reassignment of inputs in combination with the loop rather confusing.. I guess the original code could be fixed, if the zip would only be unpacked if the list constructed inside the zip is not empty.

Yes this seemed liked the simplest way to solve the 'nothing left to do' bug and also not iterate and filter the inputs twice in the same way. But certainly open to suggestions for making it more readable. Maybe just renaming the new inputs -> inputs_parsed or something?

To be honest, for me list comprehensions are much more expressive compared to list constructions using loops. However, at least the variable inputs should be renamed.

Of course generally I agree! But when for loops can do the same thing more efficiently... I think they are the correct tool to use. I've renamed the parsed inputs to inputs_contract.

mrader1248 · 2019-12-03T20:23:58Z

Can you give me an example, where it is beneficial to construct outer products?

mrader1248 · 2019-12-03T20:30:30Z

Sorry, it took me a while to have a look at this. Overall, LGTM. And especially thanks for fixing this stupid smallest dimension 1 bug.

jcmgray · 2019-12-03T21:44:43Z

Can you give me an example, where it is beneficial to construct outer products?

I've put this example in the tests (taken from you in another thread I think!):

def test_custom_dp_can_optimize_for_outer_products():
    eq = "a,b,abc->c"

    da, db, dc = 2, 2, 3
    shapes = [(da,), (db,), (da, db, dc)]

    opt1 = oe.DynamicProgramming(search_outer=False)
    opt2 = oe.DynamicProgramming(search_outer=True)

    info1 = oe.contract_path(eq, *shapes, shapes=True, optimize=opt1)[1]
    info2 = oe.contract_path(eq, *shapes, shapes=True, optimize=opt2)[1]

    assert info2.opt_cost < info1.opt_cost

Of course in practice I imagine it's almost never useful. However I think it would make sense for 'dp' to replace the optimal implementation, and if that happens then the option to be guaranteed optimal seems beneficial for testing edge cases etc. Plus for v small sizes there's no penalty hit.

@dgasmith do you have any thoughts on pointing 'optimal' to 'dp'? Maybe in a later PR.

jcmgray · 2019-12-03T22:29:08Z

@dgasmith Also happy to have a go a adding more docs (e.g. each variable etc) if you think necessary.

mrader1248 · 2019-12-04T09:16:09Z

@jcmgray I see, I thought you found an example where constructing outer products first leads to a better solution even in the asymptotic limit. Maybe it would be good to mention in the docs that allowing for intermediate outer products can lead to horrible optimisation times?

jcmgray · 2019-12-04T13:05:41Z

@jcmgray I see, I thought you found an example where constructing outer products first leads to a better solution even in the asymptotic limit

Well in the asymptotic limit where da and db are fixed but dc get's larger, indeed the outer product is still necessary here.

Maybe it would be good to mention in the docs that allowing for intermediate outer products can lead to horrible optimisation times?

I've mentioned this in the docstring for the search_outer option, but yes might be worth adding another warning to the main docs as well.

jcmgray · 2019-12-06T17:53:16Z

OK, so I've added a warning to the 'dp' docs about turning on the outer product search, and factored out the single term parsing of inputs. Unless there are more comments I'll merge in a bit.

updates to 'dp' optimizer

ed7d0ca

dgasmith reviewed Dec 2, 2019

View reviewed changes

jcmgray added 2 commits December 2, 2019 16:43

further tweaks

2945a50

update dp docs

94bfef0

dgasmith approved these changes Dec 3, 2019

View reviewed changes

mrader1248 reviewed Dec 3, 2019

View reviewed changes

add some more documentation

0ee82eb

add dp outer warning to docs and factor out single term parsing

37bc0d3

dgasmith approved these changes Dec 7, 2019

View reviewed changes

dgasmith merged commit 84a805d into dgasmith:master Dec 7, 2019

This was referenced Dec 8, 2019

Getting "not enough values to unpack" with optimize=dp #116

Closed

optimize='dp' gets confused by singleton factors #118

Closed

jcmgray deleted the dp-updates branch January 16, 2020 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updates to 'dp' optimizer #119

updates to 'dp' optimizer #119

jcmgray commented Dec 2, 2019 •

edited

Loading

codecov bot commented Dec 2, 2019 •

edited

Loading

dgasmith left a comment

dgasmith Dec 2, 2019

jcmgray Dec 2, 2019

dgasmith Dec 2, 2019

mrader1248 Dec 3, 2019

dgasmith Dec 2, 2019

jcmgray Dec 2, 2019

mrader1248 Dec 3, 2019

dgasmith left a comment

dgasmith Dec 3, 2019

dgasmith Dec 3, 2019

mrader1248 Dec 3, 2019

jcmgray Dec 3, 2019

mrader1248 Dec 4, 2019

jcmgray Dec 4, 2019

mrader1248 commented Dec 3, 2019

mrader1248 commented Dec 3, 2019

jcmgray commented Dec 3, 2019

jcmgray commented Dec 3, 2019

mrader1248 commented Dec 4, 2019

jcmgray commented Dec 4, 2019

jcmgray commented Dec 6, 2019

		@@ -197,6 +197,59 @@ def test_greedy_edge_cases():
		assert check_path(path, [(0, 1), (0, 2), (0, 1)])


		def test_dp_edge_cases_dimension_1():

updates to 'dp' optimizer #119

updates to 'dp' optimizer #119

Conversation

jcmgray commented Dec 2, 2019 • edited Loading

Description

Todos

Status

codecov bot commented Dec 2, 2019 • edited Loading

Codecov Report

dgasmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgasmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrader1248 commented Dec 3, 2019

mrader1248 commented Dec 3, 2019

jcmgray commented Dec 3, 2019

jcmgray commented Dec 3, 2019

mrader1248 commented Dec 4, 2019

jcmgray commented Dec 4, 2019

jcmgray commented Dec 6, 2019

jcmgray commented Dec 2, 2019 •

edited

Loading

codecov bot commented Dec 2, 2019 •

edited

Loading