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

Method parameter_labels() returns an empty array #456

Open
Pablovinas opened this issue Jun 24, 2024 · 1 comment
Open

Method parameter_labels() returns an empty array #456

Pablovinas opened this issue Jun 24, 2024 · 1 comment
Assignees
Labels
bug A bug or regression
Milestone

Comments

@Pablovinas
Copy link

Describe the bug
After adding operations to an explicit model constructed via pygsti.models.ExplicitOpModel(), the method parameter_labels() returns an empty array and does not recognize the model's parameters. If the model has been initially imported from pygsti.modelpacks, the array is not updated with the new parameters.

To Reproduce
Steps to reproduce the behavior:

  1. Create a model, such as those defined in the the ExplicitModel.ipynb tutorial. We can additionally define custom operations as in the CustomOperator.ipynb tutorial and keep track of our parameters after naming them using the parameter_labels(self) property.
  2. Once the model is created, use mdl.parameter_labels to get the vector of model member's parameters within the model. It returns an empty array, causing further issues such as the collect_parameters()method not working.

Expected behavior
I would expect the labels and parameters defined in the operations to be automatically recognised and updated in the model containing them.

Environment:

  • pyGSTi version 0.9.12.3
  • python version 3.12.3
  • OS: macOS Ventura 13.0.1

Additional context
I have been able to circumvent this issue by explicitly using mdl._rebuild_paramvec(). However, I believe this should be done automatically when defining the model.

@Pablovinas Pablovinas added the bug A bug or regression label Jun 24, 2024
@coreyostrove coreyostrove self-assigned this Jul 8, 2024
@coreyostrove coreyostrove added this to the 0.9.13 milestone Jul 8, 2024
@coreyostrove
Copy link
Contributor

Hi @Pablovinas, thanks for the detailed report!

This isn't a bug per se, but rather related to a fairly old design choice to make the parameter management for models update lazily. As you've identified above, as currently implemented simply assigning a new value to a model member in one of the Model object's member dictionaries doesn't actually force a rebuild (i.e. a call to _rebuild_paramvec) in and of itself. I wasn't around for the original implementation of this, but I imagine it was for performance related considerations (rebuilding the parameter vector is actually a pretty expensive operation). The downside, as you discovered, is unexpected behavior wherein the values returned for certain model attributes can fall out of sync with the correct values depending on when you make the queries. This is not a particularly great behavior to have and it has tripped me up a number of times as well, so I think this is a good opportunity to fix this.

There are a couple options I can think of on how to proceed, so I wanted to open up some discussion on this to see what other folks think.

  1. Keep things lazy, but audit Model and add additional rebuild checks for publicly facing properties where we identify a potential for post-assignment inconsistencies.
  2. Modify the behavior of OrderedMemberDict, adding a call to _rebuild_paramvec to the __setitem__ method here (
    self.parent._mark_for_rebuild(new_item)
    ).
  3. Both 1 and 2.

I suspect the answer is 3, but am open to thoughts on this. Option 2 would address the particular problem that you encountered above, but it would leave the door open to other possible avenues for desynchronization. As an example, models typically inherit their parameter labels from the parameter labels of the constituent model member objects (though not always strictly so, as in the case of parameter collection which you mentioned using above). With just the fix in option 2 it would be possible to create a desynchronization condition by querying the model's parameter labels immediately after manually updating the parameter labels for one of it's children. Option 1 would address this, but has the potential for non-trivial performance impacts if we aren't careful, so I'd want to check to make sure we aren't calling the parameter_labels property in any particularly hot loops before committing to that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug or regression
Projects
None yet
Development

No branches or pull requests

2 participants