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

Dag In/Out nodes should have op and/or names #8314

Open
nonhermitian opened this issue Jul 9, 2022 · 8 comments
Open

Dag In/Out nodes should have op and/or names #8314

nonhermitian opened this issue Jul 9, 2022 · 8 comments
Labels
mod: transpiler Issues and PRs related to Transpiler type: feature request New feature or request

Comments

@nonhermitian
Copy link
Contributor

What should we add?

Currently DAGOutNode and DAGInNode have no op or name attributes. This makes looking for specific operations more difficult than it has to be, and one must always do a type check first. For example:

https://github.com/Qiskit/qiskit-terra/blob/16169de38ba858747e796a2acb5c96f8f37cb3b9/qiskit/dagcircuit/dagcircuit.py#L1404

It would be nice if they could be set as None ops and/or be given names like in / 'out so that they can easily be directly checked.

@nonhermitian nonhermitian added the type: feature request New feature or request label Jul 9, 2022
@alexanderivrii alexanderivrii added the mod: transpiler Issues and PRs related to Transpiler label Aug 16, 2022
@nonhermitian
Copy link
Contributor Author

Bumping this as I am running into this issue once again.

@nonhermitian
Copy link
Contributor Author

In particular, they should be given a name _in_node or the like rather than none, as it makes life infinitely easier to check for nodes of a given type and name together.

@jakelishman
Copy link
Member

jakelishman commented Mar 11, 2024

Honestly, this doesn't seem like a good idea to me. The in and out nodes not being operations is an important part of the DAG structure, and operations acting on the DAG must be aware of that. If we add dummy op and name fields to DAGInNode and DAGOutNode, we make it easier to treat them as if they were ops (what you want here), but the flip side of that is that it becomes a lot easier to forget to check whether the node actually represents a node or not, which would make it a lot easier to write buggy transpiler passes without knowing.

If you want to do this in your own code, would explicitly doing

name = getattr(dag_node, "name", None)

or similar work for you? (Also, None could be dag_node.__class__.__name__ if you want a magic string name to distinguish InNode from OutNode.)

@jakelishman
Copy link
Member

DAGCircuit has methods op_nodes and topological_op_nodes that already filter out the non-DAGOpNodes, if that's helpful, too.

@nonhermitian
Copy link
Contributor Author

Hmm, let me see if those work. Basically I want to do something like check the names of a call like prev_nodes = dag.successors(node) in a manner that does not require instance and name checking

@jakelishman
Copy link
Member

Perhaps an alternative here could be to add keywords to successors and friends to limit them only to op nodes in the first place, if you don't care if they're final nodes or not, just about what operations (if any) follow them?

@alexanderivrii
Copy link
Member

We have discussed adding op_predecessors and op_successors to DAGCircuit in the past: I have even added to one of my recent PRs #11811. Though maybe it makes sense to put this into a separate PR.

@nonhermitian
Copy link
Contributor Author

That would be a nice solution as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod: transpiler Issues and PRs related to Transpiler type: feature request New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants