Skip to content

Conversation

@tonybruguier
Copy link
Contributor

@CirqBot CirqBot added the size: L 250< lines changed <1000 label Mar 17, 2022
@tonybruguier tonybruguier marked this pull request as ready for review March 17, 2022 16:15
@tonybruguier tonybruguier requested review from a team, cduck and vtomole as code owners March 17, 2022 16:15
@tonybruguier tonybruguier requested a review from maffoo March 17, 2022 16:15
@MichaelBroughton MichaelBroughton self-assigned this Mar 17, 2022
@tonybruguier
Copy link
Contributor Author

@MichaelBroughton
Do you have any comments for this PR? I think the overall approach was OK'ed during the cynq, but reconsidering is of course possible.

@vtomole
Copy link
Collaborator

vtomole commented Apr 14, 2022

The discussion from Cirq sync was about considering whether or not this should be in Cirq proper. I think someone was going to ask around internally to find out if anyone is interested in Bayesian Networks.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@tonybruguier
Copy link
Contributor Author

@vtomole
My understanding is that the present PR should be in contrib and a Jupyter that uses this code to Gibbs sampling is of interest.

But I am totally OK with re-considering. I certainly don't want to pollute the repo and could abandon this PR if it is misguided, of course.

Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

Good first pass. I'm wondering if we've got the logic from the paper exactly captured. Are we handling the cases of the root vs child nodes ? Perhaps it might be worth recreating a test to match what is done in figure 10 for corectness sake ?

Comment on lines 109 to 110
for param, init_prob in self._init_probs:
yield self._generate_got_set_for_init_prob(qubit_map[param], init_prob)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we assuming all nodes are root nodes and none follow along with eqn 14 from the paper ?

Copy link
Contributor Author

@tonybruguier tonybruguier Apr 22, 2022

Choose a reason for hiding this comment

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

Correct. We just specify the dependent nodes with a None probability. Also the user messes up and specify a node is independent (i.e. with an initial probability) and then assigns it as a dependent variable, the code will run fine (but return absurd results).

The reason I decided to do it this way, was that there are other ways to mess things up. We would have to check the graph is directed acyclic, but that seems like pulling a lot of graph code in. I can do it if you want, of course. For now, I just put a warning in the docstring.

Leaving unresolved. Let me know?

Comment on lines +96 to +153
if not isinstance(params, tuple):
raise ValueError('Conditional prob params must be a tuple.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: don't need to make type checking part of python APIs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but I want to distinguish cases like
('q0')
versus
('q0',)

The first version is NOT correct, but without the check, the code will iterate over the characters 'q' and '0'. It's a very common mistake, so this could be a good help.

@tonybruguier
Copy link
Contributor Author

tonybruguier commented Apr 21, 2022

Intermed

Good first pass. I'm wondering if we've got the logic from the paper exactly captured. Are we handling the cases of the root vs child nodes ? Perhaps it might be worth recreating a test to match what is done in figure 10 for corectness sake ?

I tried to recreate the network of figure 10, and I got this circuit:
circuit

It is simpler than figure 11 because of Gray optimization that I had added on top of the paper + a different definition of the gate. Also, note that I suspect the paper had the order of the qubits flipped.

Still, I tried to reproduce the values of table 1, and I was able to (except that the paper could have been more precise and write 0.4985 instead of 0.499)

@tonybruguier
Copy link
Contributor Author

Thanks @MichaelBroughton . Now, the PR is ready for another iteration. I left a few comments unresolved to make sure I understood what you were asking. PTAL when you can?

@tonybruguier
Copy link
Contributor Author

Friendly ping when you have the time? I understand that v1.0 takes priority, of course.

Copy link
Collaborator

@dabacon dabacon left a comment

Choose a reason for hiding this comment

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

@MichaelBroughton may want to take another look.

@tonybruguier
Copy link
Contributor Author

Thanks. PTAL.

@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 8, 2022
@MichaelBroughton MichaelBroughton added good for learning For beginners in QC, this will help picking up some knowledge. Bit harder than "good first issues" """"""""""""""""""" labels Jun 8, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 8, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented Jun 8, 2022

Automerge cancelled: A status check is failing.

@CirqBot 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 Jun 8, 2022
@tonybruguier
Copy link
Contributor Author

@MichaelBroughton
I am not sure why automerge failed, but I re-pushed without any code change and it seems OK now.

@vtomole vtomole added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 22, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 22, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented Jun 22, 2022

Automerge cancelled: A status check is failing.

@CirqBot 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 Jun 22, 2022
@vtomole
Copy link
Collaborator

vtomole commented Jun 22, 2022

@tonybruguier Can't merge due to some checks failing.

@vtomole vtomole added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 22, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 22, 2022
@CirqBot CirqBot merged commit b948a66 into quantumlib:master Jun 22, 2022
@CirqBot 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 Jun 22, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

good for learning For beginners in QC, this will help picking up some knowledge. Bit harder than "good first issues" size: L 250< lines changed <1000

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants