-
Notifications
You must be signed in to change notification settings - Fork 639
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
Initial draft of instrument support. #1687
base: dev.major
Are you sure you want to change the base?
Conversation
qutip/qinstrument.py
Outdated
except: | ||
return False |
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 will catch arbitrary exceptions. It should be more specific to catch only the required exception.
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 is a draft PR to get initial feedback on the API design; it's not intended that code in this PR is in its final working state (e.g. there's not much in the way of unit testing, the use of the walrus operator isn't compatible with some versions of Python that should be supported, and so forth).
That said, the intent of this particular function is to check the iterability of a given value without causing any other effects on execution — it would be surprising for something analogous to an isinstance
call to raise an exception, such that exceptions should not propagate out from this check.
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.
Hey @cgranade, if you planned to address this later, once the discussion on the API was finished, please, feel free to ignore this comment. I was just interested in the PR and decided to take a quick look and, since I saw the catch-all exception, I thought I could point it out to avoid it appearing in the final working version.
The reason I think it is dangerous to catch all in the try/except statement is that it could lead to undesired and inconsistent behaviour, difficult to debug. For instance, a catch-all try/except will also intercept a KeyboardInterrupt
which in turn is used to stop the execution of a program (for example in a notebook). The following code when executed in a notebook will not stop when trying to interrupt the kernel:
def _is_iterable(obj):
try:
time.sleep(1)
iter(obj)
return True
except:
return False
while True:
print(_is_iterable([1]))
Such problem seems unlikely to happen, after all, I required to include a sleep time of 1 second to reproduce the bug. However, it could happen. Such problems can be avoided by catching TypeError
instead of any exception.
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.
Probably good for us to leave this note here in order to remind us to fix this later, even if it's not important right this second. It would be bad to forget. :)
P.S. We probably only want to catch TypeError
or something like that.
@cgranade I'm really liking the overall look of the Instrument class -- it's a much easier way to deal with measurements than calling the measurement operations all the time & of combine measurement operations with other operations on Qobjs. I'm keen to hear feedback from others too, but in the mean time I'm going to note some thoughts here for when I come back to this:
Hoping to hear comments from others! |
@cgranade When we initially talked about this, we also spoke about storing Kraus superoperators with it, but it's not completely clear to me whether this route covers that case. "No" is fine (we don't have to make this work for everything) but if the answer is "Yes" or "Maybe", what would that look like? |
Thank you for the kind words!
I'll admit I've not kept up as much with the 5.0 changes as I should have, but I'm happy either way; I can definitely see the benefit to targeting 5.0 and keeping code maintenance down, or to getting the feature out for folks to use sooner at the cost of more development work.
Honestly, agreed; I tried a few different designs to try and get rid of those two classes, but they all felt a bit awkward and special-cased. Happy to revise, though, to lower the barrier to using the new feature.
My initial thought was to do something like use tuples instead of One alternative may be to have a single subclass
Sounds good, will move those out to be ordinary functions, then.
I've played around with using
Fair enough, easy to drop that as well. Would you want everything dropped, or would some things like the core type
Thanks for your review, I'll work on addressing, then excited to hear what others think as well! 💕 |
That sounds like a great example!
Let's keep QInstrument in since that feels at the same level as Qobj and QobjEvo and we can think about other things on a case by case basis (and see what v5 is doing). |
At the moment, no. Following our discussion, it sounded like separating the two features may make the most sense such that I wanted to focus first on representing instruments.
Awesome, I'll go add that as a draft PR to the notebooks repo, then, so as to develop both in parallel.
Sounds good, will do! |
14e2a92
to
ec49f6d
Compare
@hodgestar Apologies for taking as long, but went on and rebased my PR to 5.0 and addressed some of your comments. In particular, factory methods have been moved out into ordinary functions, and I added a simple string-based format for outcome labels (leaving For the notebook, I ran into the slight issue that the qutip-notebooks repo uses a more copyleft license, but in the meantime I put together a notebook at https://gist.github.com/cgranade/7c2a5a0827dddc4281666ad45763b1ec that includes a few examples of the API in use, including for modeling a simple teleportation channel. I still need to address PEP8 issues and add tests, but I think it should be a bit further along; thanks for all your help and feedback! 💕 |
@cgranade The new dimensions support has now been merged, so I think we can pick up this PR again. I also did all the running around necessary to re-license the notebooks (qutip/qutip-notebooks#146). Note that the home for new notebooks is https://github.com/qutip/qutip-tutorials. |
As discussed in #1673, this PR is an initial draft of what instrument support could look like to get feedback and discussion going on the API and functionality. As such, code style, documentation, unit testing and so forth still need to be addressed (see check list below).
Checklist
Thank you for contributing to QuTiP! Please make sure you have finished the following tasks before opening the PR.
You can use pycodestyle to check your code automatically
doc
folder, and the notebook. Feel free to ask if you are not sure.Delete this checklist after you have completed all the tasks. If you have not finished them all, you can also open a Draft Pull Request to let the others know this on-going work and keep this checklist in the PR description.
Description
This PR adds a new class,
QInstrument
, that wraps a decomposition of a quantum instrument into completely positive trace non-increasing maps (subnormalized channels). This new class can be used to predict measurement outcomes and post-measurement states for a variety of different quantum systems:Instruments can be combined through composition and tensor products (corresponding to
Seq
andPar
outcome labels, respectively):Impossible outcomes are detected and truncated automatically:
Arbitrary subsystem dims are supported:
Incomplete instruments (that is, where the probability of obtaining any result is less than 1, as in the erasure channel case) can be completed:
Measurement outcome labels can be re-indexed onto integer labels according to lexographical sorting of original labels:
Composition and tensor products can be combined:
Outcomes can be labeled by arbitrary hashable types:
Nonselective channels (that is, the channel resulting from discarding all measurement outcomes) can be efficiently obtained using the
nonselective_process
property:Trace preserving and complete positivity conditions can be checked in a similar way to
Qobj
instances:Finally, for convienence, this PR also proposes QuaEC-style shorthand for tensor products of quantum objects and instruments:
Related issues or PRs
Changelog
Adds support for quantum instruments, a generalization of measurements and channels.