Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Switched to a new dump functionality exposed by IQ#. #727

Merged
merged 2 commits into from
Jan 14, 2022

Conversation

kuzminrobin
Copy link
Contributor

@kuzminrobin kuzminrobin commented Jan 12, 2022

Simplified the Dump functionality for QuantumSimulator and the functionality use in IQSharp.
This is a part of the set of 3 PRs:
Q#RT side,
IQ#,
Katas.

@kuzminrobin kuzminrobin self-assigned this Jan 12, 2022
@kuzminrobin kuzminrobin marked this pull request as ready for review January 13, 2022 05:08
@kuzminrobin kuzminrobin changed the title Switched to a new dump functionality exposed by QuantumSimulator Switched to a new dump functionality exposed by IQ#. Jan 13, 2022
Copy link
Contributor

@cgranade cgranade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but would be good to get @tcNickolas' review as code owner as well. One thing I'll point out quickly, though, is that as written in this PR, the %kata magic command won't support histogram views for displayable states. I think that was already the case before this PR, so not a regression so much as a known limitation.

@tcNickolas
Copy link
Member

@kuzminrobin I'm not very familiar with the katas magic part of code, @anjbur usually helps me with reviews on them. Can you please provide a bit of context on this change? Are there going to be any user-facing behavior changes? What about @cgranade's comment on #688?

(Also, it looks like this PR supersedes #688 - could you please close the other PR to keep things more organized?)

@cgranade
Copy link
Contributor

@kuzminrobin I'm not very familiar with the katas magic part of code, @anjbur usually helps me with reviews on them. Can you please provide a bit of context on this change?

The long and short of it is that the full-state simulator used a different means of reporting diagnostics than other simulators (e.g.: Toffoli, open-systems). As part of developing the sparse simulator, the full-state simulator has been updated to use the same approach to diagnostics reporting, allowing a lot of duplicate code to be removed from both the qsharp-runtime and iqsharp repos. This PR makes sure that the new unified approach works here as well.

Are there going to be any user-facing behavior changes? What about @cgranade's comment on #688?

When using %simulate, a user can opt in to measurement histograms using %config dump.measurementDisplayHistogram. Unlike other diagnostics, histograms are plotted on the client-side rather than on the kernel-side so as to enable responsive layouts with chart.js. My earlier comment was to point out that the logic to send charts to the client needed to be added in order for %kata to support the same functionality, but upon further investigation, it looks like that problem exists with or without this PR, such that there's likely no regression.

@kuzminrobin
Copy link
Contributor Author

Can you please provide a bit of context on this change?

This change stops calling the functionality (in IQ#) that is being removed as a simplification of the Dump functionality in QuantumSimulator.

Are there going to be any user-facing behavior changes?

No. Nothing that the user's code could be relying on.

What about @cgranade's comment on #688?

I added a reply there that the note has been resolved in this repeated attempt.

could you please close the other PR

Done. Thanks for the reminder! ;-)

Copy link
Member

@tcNickolas tcNickolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cgranade Right, this bug! I thought initially that your comment was about the measurement probability bars shown in regular DumpMachine output, thank you for clarifying!

That was my one concern, I'll trust you folks on the syntax of the change :-) Approving, I'll let you update the branch to latest and merge when the time is right (not sure if it needs to go in sync with the other two PRs)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants