-
Notifications
You must be signed in to change notification settings - Fork 27
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
Rich wires #523
base: develop
Are you sure you want to change the base?
Rich wires #523
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #523 +/- ##
===========================================
- Coverage 90.20% 90.20% -0.01%
===========================================
Files 102 102
Lines 6321 6310 -11
===========================================
- Hits 5702 5692 -10
+ Misses 619 618 -1
Continue to review full report in Codecov by Sentry.
|
classical_out or set(), | ||
classical_in or set(), | ||
): | ||
if any( |
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.
I don't think we need this check since the type hints already make it explicit that's what we expect
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.
I think we need it because otherwise one can pass an unsorted tuple, or list. Like (3,2,1) and then the modes are screwed up. To guard against that we would have to cast to a set, but I'd rather not do that by default because then implicit stuff happens.
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.
But we type hint it as a set does that mean we should update the type hint?
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.
What change do you have in mind? Here the hint says set[int] | None
and the method raises if it isn't that.
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.
Right the type hint says its set | None
so we shouldn't be expecting something like an unsorted tuple or list to be passed in. If we want to support passing in an unsorted tuple or list then the type hint should be updated to reflect that. If not, then the check shouldn't be necessary as the type hint is already implying it is set | None
.
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.
As I'm sure you know, the type hint does not enforce anything. Raising an exception in this case guards against running into bugs because of an ordered data structure is passed in when out of order. If this is your only concern about this PR can you accept this check and approve it?
Wires have been challenging to maintain due to evolving requirements.
This PR introduces a better abstraction for Wire that stores a collection of
QuantumWire
andClassicalWire
dataclasses. Metadata can be added to these dataclasses without modifying the Wires object’s handling of indices and dictionaries. Many methods, like__matmul__
, are greatly simplified.