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

[BUG] Identical blocks might break dependecy_graph #176

Open
gwwatkin opened this issue Nov 24, 2021 · 7 comments
Open

[BUG] Identical blocks might break dependecy_graph #176

gwwatkin opened this issue Nov 24, 2021 · 7 comments
Labels
bug Something isn't working

Comments

@gwwatkin
Copy link
Member

Issue Description

This line suggests it:

If two blocks are identical they will be interpreted as the same key in this map which doesn't sounds like the desired behavior.

Proposed Solution

Need to investigate if this is the case and find a solution. Also the code in dependency_graph.py is very hard to read because it's missing type annotations. Especially because it's generic/templated code it should be annotated.

Additional References

The equivalence of rotations

def __eq__(self, other) -> bool:
return (
isinstance(other, PauliRotation)
and self.rotation_amount == other.rotation_amount
and self.ops_list == other.ops_list
)

and measurements

def __eq__(self, other) -> bool:
return (
isinstance(other, Measurement)
and self.isNegative == other.isNegative
and self.ops_list == other.ops_list
)

@gwwatkin gwwatkin added the bug Something isn't working label Nov 24, 2021
@alexnguyenn
Copy link
Member

A quick solution I can think of is to remove that method since DependencyGraph.Node has parents and children list, which is enough for us to figure out the adjacency list anyway.

@gwwatkin
Copy link
Member Author

gwwatkin commented Nov 24, 2021

It should, as we are only ever using it in a debug file to view the graph. I think working on this class is lower priority than anything in the UF epic, so I think it's fine to leave this for now, but good to have the issue logged if we do start to use it.

@gwwatkin
Copy link
Member Author

If we go back to the plan of parallelizing operations

@alexnguyenn
Copy link
Member

I think it will also break this:

for parent in s.parents:
if parent.op not in visited:
visited.add(parent.op)
queue.append(parent)

Which is part of the DAG generating method, with visited being a set. Looking through the codecov report seems like it is not covered by our test case which is why I missed it. I might need to look more into it but I think we can just use the node there instead of op (by visisted.add(parent))?

@alexnguyenn
Copy link
Member

Yeah I will fix the type annotations in there as well. They are a mess 😓

@alexnguyenn
Copy link
Member

def __eq__(self, other) -> bool:
if isinstance(other, DependencyGraph.Node):
return self.op == other.op
else:
return False

lol I override __eq__ for Node as well. Might need to get rid of it.

@gwwatkin
Copy link
Member Author

It's good we have all these notes. Don't think we need to act on it yet.

@gwwatkin gwwatkin changed the title [ANALYSIS] Identical blocks might break dependecy_graph [BUG] Identical blocks might break dependecy_graph Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants