-
Notifications
You must be signed in to change notification settings - Fork 57
FOGI model serialization and some FOGI model optimization #598
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
Conversation
@@ -88,6 +89,7 @@ def __init__(self, state_space, labels, basis_1q=None): | |||
comprise the basis element labels for the values of the | |||
`ElementaryErrorgenLabels` in `labels`. | |||
""" | |||
super().__init__() |
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.
Every object that is nicely serializable needs to have this init statement
pygsti/baseobjs/errorgenbasis.py
Outdated
return state | ||
@classmethod | ||
def from_nice_serialization(cls, state): | ||
return cls(_StateSpace.from_nice_serialization(state['state_space']), [_GlobalElementaryErrorgenLabel.cast(label) for label in state['labels']], state['_basis_1q'] if isinstance(state['_basis_1q'], str) else _Basis.from_nice_serialization(state['_basis_1q'])) |
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.
Added support for _basis_1q variables which can be strings or Basis objects according to the docstring
@@ -418,7 +418,7 @@ def create_subspace(self, labels): | |||
StateSpace | |||
""" | |||
# Default, generic, implementation constructs an explicit state space | |||
labels = set(labels) | |||
labels = sorted(set(labels)) |
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.
This change was made with @coreyostrove to make FOGI model construction deterministic
self.gauge_space = gauge_space | ||
self.elem_errorgen_labels_by_op = elem_errorgen_labels_by_op | ||
self.op_errorgen_indices = op_errorgen_indices | ||
self.fogi_directions = fogi_directions |
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.
The init method of this class used to not only create a FOGIstore object, but also compute all fogi directions. In order to be able to load them from disk, we needed a simple init method which just takes attributes and spits back the object without doing any math.
pygsti/models/fogistore.py
Outdated
self.fogv_labels = ["%d gauge action" % i for i in range(self.fogv_directions.shape[1])] | ||
|
||
@classmethod | ||
def compute_fogis(cls, gauge_action_matrices_by_op, gauge_action_gauge_spaces_by_op, errorgen_coefficient_labels_by_op, |
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.
The old __init__method became compute_fogis(). It is decorated as a classmethod such that it can be used without the existence of a fogi_store object. As it intended to be used, at the end it creates an object which is returned back to the user.
@@ -657,56 +657,6 @@ def num_modeltest_params(self): | |||
self._clean_paramvec() | |||
return Model.num_modeltest_params.fget(self) | |||
|
|||
@property |
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.
It seems like a mistake that develop had all these methods duplicated
index_mm_map[gpidx].append(obj) | ||
index_mm_label_map[gpidx].append(lbl) | ||
self._index_mm_map = index_mm_map | ||
self._index_mm_label_map = index_mm_label_map |
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.
the resulting index_mm_map was incorrect for FOGI models. This is necessary for our set_parameter_values optimization
|
||
indices = non_zero_errgens[0] | ||
values = new_errgen_vec[indices] | ||
vec_to_access = new_errgen_vec |
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.
This if statement captures which error generators are affected by the FOGI vector changing.
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.
vec_to_access is a new variable which will always hold the vector of object parameters. In the case of FOGI models, it will always correspond to the updated error generator vector.
#and c) the effect is a child of that POVM. | ||
|
||
if isinstance(self._layer_rules, _ExplicitLayerRules): | ||
updated_children = [] |
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.
unique_mms identifies which operations correspond to the error generators that are changing, which then allows us to only update those in our model inside the loop. This way we only update the objects in our model which are affected by the values that were modified by the user.
@@ -1304,52 +1253,71 @@ def set_parameter_values(self, indices, values, close=False): | |||
------- | |||
None | |||
""" | |||
orig_param_vec = self._paramvec.copy() |
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.
All of the code below assumes that parameter interposers are only used for FOGI models, and that FOGI models are built from GLND parameterizations (i.e. every FOGI quantity is defined in terms of error generators)
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.
Mostly looks good. A few requests inline. Also a general request to identify whether or not we have unit test coverage for the new functionality that is being added for both the serialization cases and the updated code path for set_parameter_values when using a FOGI model/model with interposer.
… local errgenbasis labels
…ks but does not return his result
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.
Only a few very minor requests for a) a couple one-sentence docstrings (see comments in code) and b) for another merge of develop into your branch to sync up changes from PRs merged in earlier today (there is at least one conflict/unintentional reversion that would be introduced otherwise). After that I think this is good to go.
@@ -730,3 +746,55 @@ def tearDown(self): | |||
# def test_randomize_with_unitary_raises(self): | |||
# with self.assertRaises(AssertionError): | |||
# self.model.randomize_with_unitary(1, rand_state=np.random.RandomState()) # scale shouldn't matter | |||
|
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.
All of the code below is not original to this PR, it was moved from test_nice_serialization.py
pygsti/tools/fogitools.py
Outdated
|
||
# Step 2: Construct Sigma | ||
Sigma = _np.zeros((U.shape[0], VT.shape[0])) | ||
_np.fill_diagonal(Sigma, S) |
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.
Pro-tip, use broadcasting here. It is much more efficient. This Sigma matrix is only being constructed here for the sake of the matrix multiplication on line 578, but what does matrix multiplication by a diagonal matrix, D, and some generic matrix A do? It rescales each row of A by the corresponding diagonal entry of D. If you have a vector V of the correct size (len=num_rows(A)) and you multiply it into A (using the standard multiplication operator, '*', not matrix multiplication's '@') then numpy's default behavior is to broadcast this multiplication across A, which is hard to describe concisely, but has exactly the desired effect of rescaling the rows of A by the corresponding values of V. But you'll achieve this with better computational complexity (only a quadratic number of multiplications instead of the full arithmetic cost of matrix multiplication).
This isn't a bottleneck as far as I am aware, so I'm not going to hold things up for the sake of this change, but there's never a bad time to learn about nifty numpy performance tricks.
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.
Tests are now passing, which was the last thing I was waiting on before approval. Great work, @juangmendoza19!
This resolves issue #595 by allowing FOGI models to be serialized. Now, any fogi model can be saved to disk with the method .write(''), and loaded back with pygsti.models.Model.read('').
The main problem was the the class FirstOrderGaugeInvariantStore was not nicely serializable, which is stored in FOGI models. All appropriate serializations were added to this class, and other classes used within.
Additionally, @coreyostrove and I modified model.py:set_parameter_values() to be executed faster when used with FOGI models