-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[DAGCircuit Oxidation] Port DAGNode
to Rust
#12380
Conversation
Also has beginnings of DAGCircuit, but this is not complete or wired up at all.
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 9068130816Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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.
Overall this looks good to me, thanks for doing it! I mostly had comments about our pickle handling through these cases - I think there's a fair amount of places where we could entirely elide the __getstate__
and __setstate__
calls and do it all through __getnewargs__
(and some places where we're duplicating things already).
The other super minor point is whether you think it's worth deferring to Python-space formatting in __repr__
methods. It's not as fast, but if anything is bottlenecked on __repr__
we should probably make sure it isn't, and using the Python-space formatting means everything will be consistent no matter how deep or shallow in the object hierarchy a __repr__
is called.
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 looks fine to merge to me, thanks for the changes.
Just for visibility for anyone else looking: the three-tuple return from __reduce__
here where the callable is the Self
type object can also be written as a __getnewargs__
/__getstate__
/__setstate__
triple, but the current way also works totally fine.
fn __getstate__(&self) -> isize { | ||
self._node_id | ||
} | ||
|
||
fn __setstate__(&mut self, nid: isize) { | ||
self._node_id = nid; | ||
} |
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.
Strictly these two could also be replaced with a __getnewargs__
as well, but it's unimportant - I wouldn't be surprised if they're never called at all, since I don't think we ever construct bare DAGNode
.
Summary
Ports
DAGNode
and subclasses to Rust. This is precursory to writing efficient ports of methods onDAGCircuit
in Rust, since we can avoid a lot of interpreter interaction.Details and comments
Everything is ported over with the exception of
semantic_eq
, since its implementation has a lot of interplay with components that we currently must access through the Python interpreter (e.g.Expr
,Parameter
, registers, etc.).