Bugfix Model Parameter Desync #482
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
August 30, 2024 21:11
These changes address unexpected behavior that can occur when manually adding an operation without then manually rebuilding the parameter vector. When this happens it is possible for the Model's internal attributes to fall out of sync with those of it's child objects. Now we check for the need to rebuild the parameter vector every time.
Fixes a minor bug in circuit that was caught by unit tests.
This adds a warning for when rebuilds are performed on models with nontrivial parameter bounds, which may result in manually set bounds being overwritten.
sserita
approved these changes
Sep 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes are meant to address the concerns raised in issue #456. To summarize that issue, when manually adding model members of a model's member dictionary it was previously possible to run into scenarios where the values returned for certain public facing attributes, such as the list of parameter labels, had fallen out of sync with the current constituents of the model. In this PR this has been addressed by adding additional checks to various public facing model attributes which check whether we need to rebuild the model, and performs that rebuild if needed before returning the requested value.
This specifically adds checks to the
parameters_labels,set_parameter_label,parameter_boundsandnum_modeltest_paramsproperties/methods.Also included is an unrelated bug in the
Circuitclass which got caught by unit testing.In the course of testing these changes I also discovered some unexpected and undocumented behavior related to the handling of parameter bounds during the model rebuild stage. When manually setting parameter bounds at the
Modellevel and not at theModelMemberlevel these manually specified values get overwritten during the rebuild process by whatever values (if any) are present in the constituent model members. We now have a check in the rebuild process which detects when there is a non-trivial parameter bound vector for a model, and a warning is raised alerting users to this behavior and the possible need to reset any manually specified parameter bounds again at the Model level (if appropriate).