-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add flamegraph visualizations to visualize T-complexity of algorithms #732
Conversation
I imagine we probably want to keep it separate and just point users to the external tool?
I think we need to find the mathematica notebook / whatever was used and maybe just write something ourselves? Not sure how hard it would be. |
…ame_graph and remove special casing for Adjoint
…eir Bloq versions
…rq_to_bloq_interop_improve
I'll clean it and update it soon |
@mpharrigan I've cleaned up the PR and addressed most of the pending comments. This is ready for a re-review. |
qualtran/drawing/flame_graph.py
Outdated
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
"""Classes for drawing bloqs with FlameGraph.""" |
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 relies on third party
flamegraph.pl
qualtran/drawing/flame_graph.py
Outdated
def _pretty_name(bloq: Bloq) -> str: | ||
from qualtran.serialization.bloq import _iter_fields | ||
|
||
ret = bloq.pretty_name() | ||
if bloq.pretty_name.__qualname__.startswith('Bloq.'): | ||
for field in _iter_fields(bloq): | ||
ret += f'[{_pretty_arg(getattr(bloq, field.name))}]' | ||
return ret |
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 seems like a hack around the issues discussed in #791. Can you add a description of what you'd like in the flame graph and link to the issue here. Is it the same thing we'd want in the call graph drawings?
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.
Left a detailed comment on the linked issue
qualtran/drawing/flame_graph.py
Outdated
return ret | ||
|
||
|
||
@functools.lru_cache(maxsize=None) |
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.
i'm surprised graph
is hashable since it is mutable. could be dangerous
sigma = _compute_sigma(bloq, graph) | ||
return t_counts_from_sigma(sigma) |
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.
why not t_complexity(bloq).t
?
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.
It doesn't sum up costs for rotation bloqs anymore, since we moved ceil(1.149 * log2(1.0 / eps) + 9.2)
to TComplexity.rotation_cost
and but we don't track the eps
in the TComplexity
object.
|
||
|
||
def _populate_flame_graph_data( | ||
bloq: Bloq, graph: nx.DiGraph, graph_t: nx.DiGraph, prefix: List[str] |
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.
document arguments -- prefix is modified
_t_counts_for_bloq(succ, graph_t) * graph.get_edge_data(bloq, succ)['n'] | ||
) | ||
data += curr_data * graph.get_edge_data(bloq, succ)['n'] | ||
# assert total_t_counts == succ_t_counts, f'{bloq=}, {total_t_counts=}, {succ_t_counts=}' |
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.
uncomment or remove
file_path: Union[None, pathlib.Path, str] = None, | ||
keep: Optional[Sequence['Bloq']] = _keep_if_small, | ||
**kwargs, | ||
) -> List[str]: |
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.
docstring
|
||
|
||
def get_flame_graph_svg_data( | ||
*bloqs: Bloq, file_path: Union[None, pathlib.Path, str] = None, **kwargs |
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.
docstring
qualtran/drawing/flame_graph.py
Outdated
data = get_flame_graph_data(*bloqs, **kwargs) | ||
|
||
data_file = tempfile.NamedTemporaryFile(mode='w') | ||
flame_graph_path = pathlib.Path(__file__).resolve().parent.parent / "third_party/flamegraph.pl" |
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.
I'm pretty sure this won't work if you pip install qualtran
.
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.
Do you have suggestions for what to change? can we fix this in a followup PR?
@mpharrigan I've addressed all comments, PTAL |
This PR adds tools to generate flamegraphs using then https://github.com/brendangregg/FlameGraph/blob/master/flamegraph.pl perl script. There are a couple of open questions
What's the best way to invoke the perl script from Qualtran? Right now, I just copied the
flamegraph.pl
file intoqualtran/drawing
directory but this is probably not the right way? We also need to figure out if using the tool causes any licensing issues. cc @mpharriganThe flame graphs added in this PR are only visualizing T-complexity traces (see plots below). The one in the THC paper (Figure 12) have number of qubits used on the y-axis. It's not clear to me how to generate these plots using the existing tools.
This PR also makes a BUNCH of improvements and bugfixes to cirq interop in order to make things work. We'll probably have to pull out pieces into separate PRs or have more detailed discussions. Some improvements made in this PR are
_extract_bloq_from_op
is now smarter to automatically map known operations from Cirq to Bloqs. This includes mappingControlledGate
toControlled
Bloq (this has some issues, so it's currently commented out. but the idea is to support this as well);_InverseCompositeGate
toAdjoint
and cirq gates corresponding to all bloqs inbasic_gates/
_t_complexity_
protocol is broken after the recent unification and introduction ofAdjoint
. Specifically,Adjoint
passesadjoint=True
parameter tosubbloq
but there's only one subbloq (MultiAnd
) that accepts this parameter. Thus, usingAdjoint(bloq).t_complexity()
for any other bloq results in an error because thesubbloq._t_complexity_()
does not accept anadjoint
parameter. It looks like this was merged as an idea but never followed up to add the parameter to all other Bloqs? The temporary fix in this PR is to comment out then_t_complexity_
ofMultiAnd
and rely on decomposition strategy instead.pretty_name
to a bunch of subbloqs.Some Flume Graph plots for THC (bloq native) and Hubbard model (cirq style native) are as follows:
Hubbard model Qubitization
THC