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
[XEB] Cycle depths during analysis #4278
[XEB] Cycle depths during analysis #4278
Conversation
mpharrigan
commented
Jun 30, 2021
- Make the argument optional in benchmark_
- If provided, you can do a subset of those in sampled_df
- More robust checking that it matches sampled_df
- Make optional - If provided, you can do a subset of those in sampled_df - More robust checking that it matches sampled_df
if len(cycle_depths) == 0: | ||
raise ValueError("`cycle_depths` should be a non-empty array_like") | ||
not_in_sampled = np.setdiff1d(cycle_depths, sampled_cycle_depths) | ||
if len(not_in_sampled) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just if not_in_sampled:
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know some style guides disagree, but I think in Cirq and me personally we usually are explicit. I think it reads closer to the intent
f"The `cycle_depths` provided include some not " | ||
f"available in `sampled_df`: {not_in_sampled}" | ||
) | ||
sim_cycle_depths = np.intersect1d(cycle_depths, sampled_cycle_depths) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the interesect1d redundant here? sampled_cycle_depths is always a superset of cycle_depths because of the previous condition, and result is always cycle_depths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that's true. removed
Automerge cancelled: A required status check is not present. Missing statuses: ['Changed files test', 'Doc test'] |
Automerge cancelled: A required status check is not present. Missing statuses: ['Changed files test', 'Doc test'] |
Beep boop |
- Make the argument optional in benchmark_ - If provided, you can do a subset of those in sampled_df - More robust checking that it matches sampled_df