Skip to content

Add a convenience method that runs both RB and XEB#6471

Merged
NoureldinYosri merged 8 commits intomainfrom
rb_xeb
Feb 22, 2024
Merged

Add a convenience method that runs both RB and XEB#6471
NoureldinYosri merged 8 commits intomainfrom
rb_xeb

Conversation

@NoureldinYosri
Copy link
Copy Markdown
Collaborator

@NoureldinYosri NoureldinYosri commented Feb 21, 2024

No description provided.

@CirqBot CirqBot added the size: M 50< lines changed <250 label Feb 21, 2024
@NoureldinYosri NoureldinYosri marked this pull request as ready for review February 21, 2024 18:53
@NoureldinYosri NoureldinYosri requested review from dabacon and eliottrosenberg and removed request for dabacon February 21, 2024 18:53
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.75%. Comparing base (e76702f) to head (eb9c0ed).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6471      +/-   ##
==========================================
- Coverage   97.75%   97.75%   -0.01%     
==========================================
  Files        1105     1105              
  Lines       94897    94909      +12     
==========================================
+ Hits        92771    92782      +11     
- Misses       2126     2127       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Collaborator

@eliottrosenberg eliottrosenberg left a comment

Choose a reason for hiding this comment

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

LGTM, although I think it would be good to add clearer documentation, either here or in InferredXEBResult about what the different types of two-qubit error rates mean, something to explain to the user what two_qubit_pauli_error, inferred_pauli_error, inferred_decay_constant, and inferred_xeb_error all mean.

Comment thread cirq-core/cirq/experiments/two_qubit_xeb_test.py Outdated
Comment thread cirq-core/cirq/experiments/two_qubit_xeb.py Outdated
Comment on lines +287 to +288
np.random.seed(0)
random.seed(0)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please remove here and in test_parallel_two_qubit_xeb above.

Both tests pass random_state=0 so the global seeds should not matter.
To the contrary if we see any flakiness w/r to global seeds here that would show something is wrong with random_state handling.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

removed them, I forgot that I already figuredout why the methods produce different results for the same seed. the reason isn't treating random_state in a wrong way but that some of the subroutines are parallized. so the order of parallization changes the results. however the tests are not flaky I wrote them to be resilient to that.

@pavoljuhas
Copy link
Copy Markdown
Collaborator

pavoljuhas commented Feb 21, 2024

gpylint run with the new configuration found following docstring issues.
I think it is worthwhile to fix them so we have a complete info from pydoc or if this gets rendered in web docs, WDYT?

************* File cirq-core/cirq/experiments/two_qubit_xeb.py
cirq-core/cirq/experiments/two_qubit_xeb.py:1:0: missing-module-docstring
cirq-core/cirq/experiments/two_qubit_xeb.py:71:0: g-doc-return-or-yield
cirq-core/cirq/experiments/two_qubit_xeb.py:99:0: g-doc-return-or-yield
cirq-core/cirq/experiments/two_qubit_xeb.py:149:0: g-short-docstring-punctuation
cirq-core/cirq/experiments/two_qubit_xeb.py:149:0: g-doc-return-or-yield
cirq-core/cirq/experiments/two_qubit_xeb.py:242:0: g-doc-return-or-yield
cirq-core/cirq/experiments/two_qubit_xeb.py:271:0: g-doc-args
cirq-core/cirq/experiments/two_qubit_xeb.py:272:0: g-doc-return-or-yield

Copy link
Copy Markdown
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

Please see comments, otherwise LGTM.

@NoureldinYosri
Copy link
Copy Markdown
Collaborator Author

@pavoljuhas I completed the missing docstrings with the help of an LLM :D. ptal

@pavoljuhas
Copy link
Copy Markdown
Collaborator

@pavoljuhas I completed the missing docstrings with the help of an LLM :D. ptal

Looks good! :-o
There is one last thing about changing the default value of the random_state argument to None, otherwise LGTM.

@NoureldinYosri NoureldinYosri enabled auto-merge (squash) February 22, 2024 01:16
@NoureldinYosri NoureldinYosri merged commit 598c84f into main Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: M 50< lines changed <250

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants