Change return type of build_call_graph to set | dict #1356
Conversation
dstrain115
commented
Aug 28, 2024
dstrain115
commented
- Changes build_call_graph to return a Union of Dict[Bloq, Union[int, Expr]] and Set[BloqCountT]
- Adds checks in main call sites to check both cases.
- Changes build_call_graph to return a Union of Dict[Bloq, Union[int, Expr]] and Set[BloqCountT] - Adds checks in main call sites to check both cases.
mpharrigan
left a comment
mpharrigan
left a comment
There was a problem hiding this comment.
this is generally what I'm looking for 👍
mpharrigan
left a comment
mpharrigan
left a comment
There was a problem hiding this comment.
consider using Counter, and remove the print statements; but after that lgtm
| from qualtran import Bloq, CompositeBloq, DecomposeNotImplementedError, DecomposeTypeError | ||
|
|
||
| BloqCountT = Tuple[Bloq, Union[int, sympy.Expr]] | ||
| BloqCountDictT = Dict[Bloq, Union[int, sympy.Expr]] |
There was a problem hiding this comment.
We could perhaps use Mapping here instead of dict? As build_call_graph is an internal function, would make it easier for bloq authors to return any mapping (eg. Counter instead of casting to dict for mypy).
There was a problem hiding this comment.
Good idea, done.
qualtran/_infra/adjoint.py
Outdated
| counts[bloq.adjoint()] += n | ||
| return counts | ||
| else: | ||
| return {bloq.adjoint(): n for bloq, n in sub_cg.items()} |
There was a problem hiding this comment.
Should we be worried about a situation where different bloqs return the same adjoint()? I think it should never happen, but am not so sure. (same concern for bloq.controlled()).
There was a problem hiding this comment.
that's an interesting corner case to consider; and the type of thing we need to be concerned about now that we are being strict about having only unique bloqs in the callees. I don't think it should happen. I defer to @dstrain115 on whether he thinks it's worth defending against that vs blocking progress on the overall migration
There was a problem hiding this comment.
I've added the Counter for both sets and dicts, which should alleviate the concern. Will push tomorrow unless there are any further issues.
anurudhp
left a comment
anurudhp
left a comment
There was a problem hiding this comment.
one minor concern about uniqueness of adjoints/controlleds, otherwise lgtm!
3 participants
