Add a rust iterator implementation for a dag's nodes_on_wire#15999
Add a rust iterator implementation for a dag's nodes_on_wire#15999mtreinish wants to merge 3 commits intoQiskit:mainfrom
Conversation
|
One or more of the following people are relevant to this code:
|
The nodes_on_wire DAGCircuit rust method was needlessly allocating the result into an intermediate vector. This was mostly an artifact of the python interface which was needs to return a list to produce an iterator (assuming the api doesn't actually return a list). But, in a rust context we don't want to eagerly create a vector. The only current rust space consumer is a pass that is just iterating over the nodes and the vector is just adding extra overhead. This commit adds a new iterator method that does the iteration over the nodes without any allocation. The current method is left in place because it is used by the python interface and also it doesn't panic if given an invalid wire and has an option to only include op nodes. While this new method just returns the iterator or panics. This also updates the sole use of the DAGCircuit::nodes_on_wire() method from rust since this method is being added for rust consumers to avoid an unnecessary allocation.
17aab62 to
2cbc74a
Compare
jakelishman
left a comment
There was a problem hiding this comment.
In principle I think this is sound and a good idea, but I would rather just make nodes_on_wire the iterator natively than largely duplicating its logic.
I think the only_ops flag could be folded into the Iterator impl, and the argument about the Python-space panics is to me not really valid, because Python space first hash to resolve a Python-space object into a valid Wire, so we know what it passes is valid. The only way it can't is if the DAG's trackers are inconsistent, in which case there'd be no guarantee we wouldn't get a panic from a dodgy qubit_io_map entry or something instead.
|
I did it in the iterator implementation originally but I didn't really like how complex it made the iterator. I settled on this because it was quite subtle. I can adjust this to replace the rust implementation with just the iterator version and move the As for the panics I'll admit I didn't look too closely at it, I just looked at what the rust function did when passed an invalid wire and felt it was unnecessary for the rust method and didn't want to preserve that because it felt unnecessary. |
This commit replaces the previously left intact rust method that built an inner vec and replaces it with the new iterator returning method. There wasn't really a need to keep the old method around as what was really needed was just to preserve the python interface which was trivial to do using the iterator in the python interface methods.
|
I replaced the rust vec returning method with the new iterator method: 8927b16 I just opted to do the filtering in the two python space methods that call the inner rust method. |
The nodes_on_wire DAGCircuit rust method was needlessly allocating the result into an intermediate vector. This was mostly an artifact of the python interface which was needs to return a list to produce an iterator (assuming the api doesn't actually return a list). But, in a rust context we don't want to eagerly create a vector. The only current rust space consumer is a pass that is just iterating over the nodes and the vector is just adding extra overhead. This commit adds a new iterator method that does the iteration over the nodes without any allocation. The current method is left in place because it is used by the python interface and also it doesn't panic if given an invalid wire and has an option to only include op nodes. While this new method just returns the iterator or panics.
This also updates the sole use of the DAGCircuit::nodes_on_wire() method from rust since this method is being added for rust consumers to avoid an unnecessary allocation.
AI/LLM disclosure