Expand SWEEP_LIKE to work for {'t': [0, 1, 3]} #2786

crystalzhaizhai · 2020-02-24T08:47:20Z

vtomole

Add test in sweepable_test.py

…plike

vtomole

Looks like you coded your own "cartesian-producter". Let's use itertools instead. Reference https://stackoverflow.com/a/533917/5716192.

vtomole · 2020-02-25T20:14:27Z

From (https://stackoverflow.com/a/40623158/5716192). Let me know what you think.

import itertools

def dict_product(dicts):
    return (dict(zip(dicts, x)) for x in itertools.product(*dicts.values()))

print(list(dict_product({'t': [0, 2, 3]}))) # [{'t': 0}, {'t': 2}, {'t': 3}]
print(list(dict_product({'t': [0, 1], 's': [2, 3], 'r': 4}))) # Will not work for this call. Handle this case because '4' is not a list

crystalzhaizhai · 2020-02-25T23:38:44Z

From (https://stackoverflow.com/a/40623158/5716192). Let me know what you think.

import itertools

def dict_product(dicts):
    return (dict(zip(dicts, x)) for x in itertools.product(*dicts.values()))

print(list(dict_product({'t': [0, 2, 3]}))) # [{'t': 0}, {'t': 2}, {'t': 3}]
print(list(dict_product({'t': [0, 1], 's': [2, 3], 'r': 4}))) # Will not work for this call. Handle this case because '4' is not a list

We recreated lots of packages by ourselves, I don't know what external packages we are supposed to use.

vtomole · 2020-02-26T01:54:35Z

@crystalzhaizhai Let's use itertools here. I bet it's more efficient than what we are doing now.

crystalzhaizhai · 2020-02-27T02:38:48Z

@crystalzhaizhai Let's use itertools here. I bet it's more efficient than what we are doing now.

It is more efficient. However, it looks like it won't work for our test case. And it also will break our previous cases. Because it only accepts lists as values.

vtomole · 2020-02-27T16:25:17Z

That code snippet was just an example of how we should use itertools. As @Strilanc said,
Check if the input is a dictionary with list values (and possibly singletons too), and if so take the cartesian product of the various entries and turn it into a sweep.

The else will deal with previous cases.

dstrain115 · 2020-03-03T18:12:19Z

cirq/study/sweepable.py

@@ -55,7 +55,24 @@ def to_sweeps(sweepable: Sweepable) -> List[Sweep]:
    if isinstance(sweepable, Sweep):
        return [sweepable]
    if isinstance(sweepable, dict):
-        return [_resolver_to_sweep(ParamResolver(cast(Dict, sweepable)))]
+        expandsweepable = [cast(dict, {})]


Worth a comment to explain what case this code is handling?

Added "change dictionary of lists to list of dictionaries of single values using Cartesian product."

cirq/study/sweepable.py

dstrain115 · 2020-03-03T18:21:24Z

cirq/study/sweepable_test.py

@@ -83,6 +83,18 @@ def test_to_sweeps_iterable_sweeps():
    assert cirq.study.to_sweeps(sweeps) == sweeps


+def test_to_sweeps_dictionary_of_list():
+    assert cirq.study.to_sweeps({'t': [0, 2, 3]}) == \


You can use parentheses to avoid the ""

assert (cirq.study.to_sweeps({'t': [0, 2, 3]}) == cirq.study.to_sweeps([{'t': 0}, {'t': 2}, {'t': 3}]))

To keep consistent with previous test cases, I changed it to parentheses. However, check/format-incremental suggests something I feel a bit ugly. Is this what we want?

- assert (cirq.study.to_sweeps({'t': [0, 2, 3]}) == - cirq.study.to_sweeps([{'t': 0}, {'t': 2}, {'t': 3}])) - assert (cirq.study.to_sweeps({'t': [0, 1], 's': [2, 3], 'r': 4}) == - cirq.study.to_sweeps([ - {'t': 0, 's': 2, 'r': 4}, - {'t': 0, 's': 3, 'r': 4}, - {'t': 1, 's': 2, 'r': 4}, - {'t': 1, 's': 3, 'r': 4}, - ])) + assert (cirq.study.to_sweeps({'t': [0, 2, 3]}) == cirq.study.to_sweeps([{ + 't': + 0 + }, { + 't': + 2 + }, { + 't': + 3 + }])) + assert (cirq.study.to_sweeps({ + 't': [0, 1], + 's': [2, 3], + 'r': 4 + }) == cirq.study.to_sweeps([ + { + 't': 0, + 's': 2, + 'r': 4 + }, + { + 't': 0, + 's': 3, + 'r': 4 + }, + { + 't': 1, + 's': 2, + 'r': 4 + }, + { + 't': 1, + 's': 3, + 'r': 4 + }, + ]))

You can use parentheses to avoid the ""

assert (cirq.study.to_sweeps({'t': [0, 2, 3]}) == cirq.study.to_sweeps([{'t': 0}, {'t': 2}, {'t': 3}]))

dstrain115 · 2020-03-03T18:23:44Z

cirq/study/sweepable_test.py

+def test_to_sweeps_dictionary_of_list():
+    assert cirq.study.to_sweeps({'t': [0, 2, 3]}) == \
+        cirq.study.to_sweeps([{'t': 0}, {'t': 2}, {'t': 3}])
+    assert cirq.study.to_sweeps({'t': [0, 1], 's': [2, 3], 'r': 4}) == \


I think this case is quite ambiguous. You are assuming that this is a product, but it could be a zip, i.e.

cirq.study.to_sweeps([ {'t': 0, 's': 2, 'r': 4}, {'t': 1, 's': 3, 'r': 4}, ])

At the very least, we need to document that multiple lists will be interpreted as a Product and not a Zip.

Agree. That should be more clear. Do you mind pointing me to the location of the intended change?

dstrain115 · 2020-03-03T18:27:49Z

cirq/study/sweepable.py

+                    for shortdict in expandsweepable:
+                        tempdict = shortdict.copy()
+                        tempdict[key] = item
+                        tempsweepable.append(cast(dict, tempdict))


The above code is quite tricky to understand. It took me a while to figure out what this was doing, since it is repeatedly assigning the values. I would echo Victory's comment for using itertools if possible. This will also avoid recreating the entire dictionary every time there is a new list found as well.

The above code is quite tricky to understand. It took me a while to figure out what this was doing, since it is repeatedly assigning the values. I would echo Victory's comment for using itertools if possible. This will also avoid recreating the entire dictionary every time there is a new list found as well.

I agree using itertools is easier to read. Do you have insights which approach would run faster? I tried to profile both methods, but the test case need to be large enough to make a difference.

vtomole · 2020-03-20T23:25:26Z

cirq/study/sweepable.py

@@ -55,7 +56,21 @@ def to_sweeps(sweepable: Sweepable) -> List[Sweep]:
    if isinstance(sweepable, Sweep):
        return [sweepable]
    if isinstance(sweepable, dict):
-        return [_resolver_to_sweep(ParamResolver(cast(Dict, sweepable)))]
+        """


Remove this docstring and replace it with #.

vtomole

LGTM

crystalzhaizhai added 9 commits February 23, 2020 23:19

adding polymorphism to to_sweeps

fc6f291

adding polymorphism to to_sweeps

a7f8c6b

adding polymorphism to to_sweeps

0c418ce

adding polymorphism to to_sweeps

23ffa3f

adding polymorphism to to_sweeps

271424f

adding polymorphism to to_sweeps

3ae0356

adding polymorphism to to_sweeps

9f09181

adding polymorphism to to_sweeps

bc5c804

adding polymorphism to to_sweeps

1b7f714

googlebot added the cla: yes Makes googlebot stop complaining. label Feb 24, 2020

Merge branch 'master' into sweeplike

cbd919a

crystalzhaizhai requested a review from vtomole February 24, 2020 08:51

vtomole requested changes Feb 24, 2020

View reviewed changes

vtomole changed the title ~~Sweeplike~~ Expand SWEEP_LIKE to work for {'t': [0, 1, 3]} Feb 24, 2020

crystalzhaizhai added 5 commits February 24, 2020 23:09

adding test to sweepable

c6662f4

adding test to sweepable

b2834e2

adding test to sweepable

ff61e6e

adding test to sweepable

fc0c7dd

Merge branch 'sweeplike' of github.com:crystalzhaizhai/Cirq into swee…

9c3d4f7

…plike

vtomole requested changes Feb 25, 2020

View reviewed changes

dstrain115 reviewed Mar 3, 2020

View reviewed changes

crystalzhaizhai added 3 commits March 15, 2020 00:11

use itertools

e006d90

fixing lint

84a01d0

delete profiling files

759ae5c

vtomole requested changes Mar 20, 2020

View reviewed changes

crystalzhaizhai added 2 commits March 27, 2020 16:39

fixing the type problem

9b55065

fix comment

011df06

vtomole approved these changes Mar 27, 2020

View reviewed changes

vtomole added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Mar 27, 2020

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Mar 28, 2020

Merge branch 'master' into sweeplike

5c131f4

CirqBot merged commit 6c57f2e into quantumlib:master Mar 28, 2020

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Mar 28, 2020

kevinsung mentioned this pull request Aug 21, 2020

Expand SWEEP_LIKE to work for {'t': [0, 1, 3]} #2698

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand SWEEP_LIKE to work for {'t': [0, 1, 3]} #2786

Expand SWEEP_LIKE to work for {'t': [0, 1, 3]} #2786

crystalzhaizhai commented Feb 24, 2020 •

edited by vtomole

vtomole left a comment

vtomole left a comment

vtomole commented Feb 25, 2020

crystalzhaizhai commented Feb 25, 2020

vtomole commented Feb 26, 2020

crystalzhaizhai commented Feb 27, 2020

vtomole commented Feb 27, 2020

dstrain115 Mar 3, 2020

crystalzhaizhai Mar 15, 2020

dstrain115 Mar 3, 2020

crystalzhaizhai Mar 15, 2020

dstrain115 Mar 3, 2020

crystalzhaizhai Mar 15, 2020

dstrain115 Mar 3, 2020

crystalzhaizhai Mar 15, 2020

vtomole Mar 20, 2020

vtomole left a comment

Expand SWEEP_LIKE to work for {'t': [0, 1, 3]} #2786

Expand SWEEP_LIKE to work for {'t': [0, 1, 3]} #2786

Conversation

crystalzhaizhai commented Feb 24, 2020 • edited by vtomole

vtomole left a comment

Choose a reason for hiding this comment

vtomole left a comment

Choose a reason for hiding this comment

vtomole commented Feb 25, 2020

crystalzhaizhai commented Feb 25, 2020

vtomole commented Feb 26, 2020

crystalzhaizhai commented Feb 27, 2020

vtomole commented Feb 27, 2020

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

Choose a reason for hiding this comment

vtomole left a comment

Choose a reason for hiding this comment

crystalzhaizhai commented Feb 24, 2020 •

edited by vtomole