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

Bugfix for Germ Selection Line Labels and Instrument Specifier Type Error Bug #508

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

pcwysoc
Copy link

@pcwysoc pcwysoc commented Nov 27, 2024

Bugfix for #507 and instrument specifier type error bug.

@pcwysoc pcwysoc requested review from a team as code owners November 27, 2024 18:41
@pcwysoc pcwysoc self-assigned this Nov 27, 2024
Copy link
Contributor

@coreyostrove coreyostrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor issue with an errant print statement, and a question for you on the conventions surrounding the change in processorspec.py. See comments below.

@@ -405,8 +405,8 @@ def instrument_specifier(self, name):
-------
str or dict
"""
if name in self.nonstd_instruments:
return self.nonstd_instruments[name]
if tuple(name) in self.nonstd_instruments.keys():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the convention here that the keys of nonstd_instruments are always tuples? If other hashable types are allowed this should be written so as to accept those options too. One way to do so would be to use the get method for dictionaries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I can answer this. I took a look at the unit test that was failing on this branch and it confirms that there was and intention for these to also have the option for string valued keys. See here.

We could rewrite this to account for both options at the level of this method, but the thing is it should be necessary to cast the name to a tuple here. If you're fixing this because you ran into a problem here it suggests to me that something upstream of this method is using malformed inputs, or has inadvertently hardcoded in the expectation that this is always a string (and not a tuple). My preference would be to track this down, but if folks would rather patch this to accept both strings and tuples (casting if needed) to get this working I won't push back too hard.

Copy link
Author

@pcwysoc pcwysoc Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coreyostrove What would a string valued key look like? The tuple valued key is ('Iz', 'Q0') for example. Currently, the problem is cropping up from processor_spec.instrument_names returns (['Iz', 'Q0'],) which is neither of the acceptable formats. This ultimately breaks QILGST.

pygsti/algorithms/germselection.py Outdated Show resolved Hide resolved
Corey Ostrove added 7 commits December 16, 2024 20:41
Changes the default behavior of casting with the 'auto' keyword to use the map forward simulator.

Update germ selection to automatically convert to a matrix forward simulator as needed.
Fix tests that were broken by the switch to using map as the default forward simulator. Either update tests to support map natively, or for ones that rely on matrix make sure that is the simulator being used.
Remove a few unit tests that made reference or relied on an old model method that no longer exists. These tests were already being skipped (or had the sections referencing this old method commented out), so this doesn't actually affect anything other than making stuff cleaner.
Start the process of fleshing out missing documentation related to processor specification requirements for state preparations and POVMs, and also introduce some minor updates to the model construction code to allow for more general application to qudit models, and more synched up string specifier conventions.
Continue the quest to properly document the convention expected for various non-standard operation specifiers. In this commit there are additional details added regarding instruments specifically. There are also changes made to the implementation of the instrument builder in the model construction code which fills in gaps between the supported specifier options for preps and POVM effects to ensure parity/consistency. Also does away with some limitations in the specifier length when using 2-tuples of prep and effect specifiers.
@coreyostrove coreyostrove requested review from a team and rileyjmurray as code owners December 18, 2024 06:20
@coreyostrove
Copy link
Contributor

Hey @pcwysoc and anyone else tracking this PR, I've just pushed some changes to this branch unceremoniously hijacking it with some scope creep.

The quest to answer the innocuous seeming question you posed above "What would a string valued key look like?" brought to bear the fact that this part of the code regarding the expected structure/formats for nonstandard operation specifiers (i.e. the contents of things like nonstd_preps, nonstd_povms, etc., was almost entirely undocumented, and so to answer that question required reading and reverse engineering a bunch of model constructor code. Since while I was in there the sunk cost was already paid and I've been frustrated by the lack of documentation before I've gone ahead and written up detailed documentation for the processor spec classes explaining in detail exactly what the specifier formats for preps, POVM and instruments that are presently supported for the code are and how they work (the gate specifiers were already reasonably well documented, so that hasn't changed). There were a whole bunch of options supported I had no idea existed.

I've also made some more substantive changes as well, introducing some additional (previously unsupported) specifier options to improve the consistency across the various nonstandard operations. E.g. Previously string-based effect specifiers (which I didn't know existed before this exercise) only supported strings of the form "E" where is some integer to be parsed into the correctly based number representing a particular standard basis state. But for preps we also supported another format for such strings where we specify the bitstring directly. This has been updated so that effects and preps have similar behavior. There are other changes along these lines that have been made.

I can also now fairly confidently answer your question:

What would a string valued key look like? The tuple valued key is ('Iz', 'Q0') for example. Currently, the problem is cropping up from processor_spec.instrument_names returns (['Iz', 'Q0'],) which is neither of the acceptable formats. This ultimately breaks QILGST.

The use of tuples for instrument names or as keys in the nonstd_instruments dictionary for a processor spec doesn't look to be an intended usage/convention, so actually these keys should likely always be strings. I suspect that the use of tuples as keys was identified by someone at some point as a hack which helped force target qubit labels for instruments into models constructed from instrument containing processor specs, circumventing the fact that availability resolution isn't currently implemented for instruments. (This is the line of code being exploited to make this hack work in case you're curious

key = _label.Label(instrument_name, inds)
).

Unfortunately what this means is that in my opinion the only proper way to fix this (without nested layers of duct-tape) is to pull the trigger on implementing availability resolution support for instruments (obviating the need for the tuple key hack). That'll probably be a pain, but fwiw we'd already identified this as something which would eventually need to happen #417.

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

Successfully merging this pull request may close these issues.

2 participants