Greedy term grouping #754
Greedy term grouping #754
Conversation
pyquil/operator_estimation.py
Outdated
| return diagonal_sets | ||
|
|
||
|
|
||
| def _commuting_sets_by_zbasis_tomo_expt(tomo_expt: TomographyExperiment): |
There was a problem hiding this comment.
what does this function name mean
There was a problem hiding this comment.
Renamed this function to _max_tpb_overlap(..), and moved the other function formerly named so into this one.
pyquil/operator_estimation.py
Outdated
| # loop through dict items | ||
| for key, es_list in diagonal_sets.items(): | ||
| # determine if key is diagonal in the same tpb as expt_setting | ||
| b_diagonal_tpb = _expt_settings_diagonal_in_tpb(key, expt_setting) |
There was a problem hiding this comment.
better variable name than key?
There was a problem hiding this comment.
put this line in an if to avoid nebulously named b_diagonal_tpb?
pyquil/operator_estimation.py
Outdated
| diagonalizing_in_term = _get_diagonalizing_basis([es.in_operator for es in updated_es_list]) | ||
| diagonalizing_out_term = _get_diagonalizing_basis([es.out_operator for es in updated_es_list]) | ||
| # update the diagonalizing basis (key of dict) if necessary | ||
| if len(diagonalizing_in_term) > len(key.in_operator) or len(diagonalizing_out_term) > len(key.out_operator): |
There was a problem hiding this comment.
this "or" seems dangerous. What if one is larger and the other is (much) smaller
There was a problem hiding this comment.
It can't be true that one is larger and the other smaller. In fact, neither can be smaller than their respective counterparts. I've added assert checks to ensure this. The "or" is basically just saying to update the tpb if either the in-term or out-term can't be measured with the current tpb; the idea being that if either of those checks fail, the experiment as a whole can't be carried out in the same tpb.
pyquil/operator_estimation.py
Outdated
| return diagonal_sets | ||
|
|
||
|
|
||
| def group_experiments_greedy(tomo_expt: TomographyExperiment): |
There was a problem hiding this comment.
on the subject of grouping: try to organize the functions within this file:
- similar / analogous functionality (ie the two "group" functions) should live near each other
- try to order it so things only call functions defined above them culminating in the meaty user-facing functions at the bottom or at the bottom of a group of related functionality (use your judgement)
There was a problem hiding this comment.
I've re-organized as follows:
- At the very top sit the classes (ExperimentSetting, etc.) and any private helper functions they need
- The class definitions are followed by some functionality converting operator_estimation objects to/from JSONs
- In the middle is the user-facing function
measure_observables(..)immediately preceded by its private helper functions - Towards the end sit the user-facing functions for grouping, similarly preceded by private helper functions needed to define them
…ExperimentSetting it's being updated for
# Conflicts: # pyquil/operator_estimation.py
|
@mpharrigan Changes (from merge) look good! |
Greedy grouping method for TomographyExperiments.
fixes #718