Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Feb 22, 2024
Merged

Conversation

NoureldinYosri
Copy link
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
Copy link

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
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 on lines 287 to 288
np.random.seed(0)
random.seed(0)
Copy link
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
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
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
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
Collaborator Author

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

@pavoljuhas
Copy link
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
37 checks passed
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
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants