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

feat!: Use a common struct for both QPU and QVM execution results #223

Merged
merged 79 commits into from
Feb 16, 2023

Conversation

MarquessV
Copy link
Contributor

@MarquessV MarquessV commented Nov 22, 2022

The original version of this PR attempted to abstract away the differences between QVM memory and QPU readout data by fitting them both into a common representation, where each register name mapped to a corresponding rectangular 2d-matrix where-in each row corresponded to the final state of that register after that shot.

It turns out QPU readout data is more complicated in that it treats each memory reference as a stream. These streams aren't delimited per-shot, so if the program isn't always performing exactly one readout to each memory reference per shot, it isn't clear from the readout data alone what the last value in each shot is. This is often the case for programs using features like mid-circuit measurement and dynamic control flow where a measure to a memory reference could occur more than once, or be skipped conditionally.

After some discussion internally, it became clear that the rectangular matrix this PR was originally attempting to build was indeed the preferred form, as it fits the type of math that is generally done on it. However, we can't escape from the realities of dealing with QPU readout data. In light of this, we've chosen to keep the abstraction, but only allow it if we can build it using the underlying readout data without making potentially erroneous assumptions. This gives users what they want in many cases, but forces them to build exactly what they need if their program calls for it.

This PR now stores the original data returned from either the QPU or QVM in a ResultData enum. Users can choose to turn this into a RegisterMap, which will build the "ideal" mapping of register names to rectangular matrices. If the underlying QPU data would be jagged, an error is returned, signaling to the user that they should build their own based on the program that was run.

The matrices themselves are built on the ndarray crate. This is not only convenient for working with the data in Rust, but it can also be passed to and from Python as a numpy ndarray using the numpy crate.

Closes #215

Copy link
Contributor

@Shadow53 Shadow53 left a comment

Choose a reason for hiding this comment

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

Really good work here!

I nitpicked a handful of things for you to address at your leisure.

The main non-nit I found is that it looks like your bounds checks suffer from an off-by-one error. Where possible, bounds-checked direct indexing should be replaced with a method that returns an Option instead.

Overall, really good stuff, and I think it'll streamline how people interact with returned data.

crates/lib/Cargo.toml Outdated Show resolved Hide resolved
crates/lib/examples/parametric_compilation.rs Outdated Show resolved Hide resolved
crates/lib/examples/parametric_compilation.rs Outdated Show resolved Hide resolved
crates/lib/examples/quil_t.rs Outdated Show resolved Hide resolved
crates/lib/src/executable.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Shadow53 Shadow53 left a comment

Choose a reason for hiding this comment

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

  • One style suggestion
  • One note about why ndarray doesn't have get_row(index) -> Option, etc. methods
  • One response to the question of using usize vs u64 for MemoryReference::index in quil-rs

crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
@MarquessV MarquessV changed the title Expose QPU and QVM execution results through a single struct feat!: Use a common struct for both QPU and QVM execution results Nov 23, 2022
@MarquessV MarquessV marked this pull request as ready for review November 23, 2022 18:23
Copy link
Contributor

@notmgsk notmgsk left a comment

Choose a reason for hiding this comment

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

Looking good. Need to spend some more time with it, when I'm not tired :)

crates/lib/Cargo.toml Show resolved Hide resolved
crates/lib/src/executable.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/api.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/qpu/client.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jselig-rigetti jselig-rigetti left a comment

Choose a reason for hiding this comment

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

LGTM, just some pyi questions

crates/python/qcs_sdk/_execution_data.pyi Outdated Show resolved Hide resolved
crates/python/qcs_sdk/_execution_data.pyi Outdated Show resolved Hide resolved
crates/python/qcs_sdk/qpu/result_data.pyi Outdated Show resolved Hide resolved
@MarquessV MarquessV dismissed stale reviews from Shadow53 and kalzoo February 14, 2023 22:24

stale

Copy link
Contributor

@kalzoo kalzoo left a comment

Choose a reason for hiding this comment

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

It's almost there! This is excellent work.

Some small typos and a note re: visibility; otherwise, this is ready to go.

crates/lib/src/executable.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Show resolved Hide resolved
crates/lib/src/qpu/result_data.rs Outdated Show resolved Hide resolved
crates/lib/src/qvm/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Shadow53 Shadow53 left a comment

Choose a reason for hiding this comment

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

Left a handful of stylistic comments and a question or two.

Main reason for requesting changes is that I ran cargo check --all-targets --all-features locally and it surfaced some errors inside of tests. It may also be worth running cargo clippy with the same flags, just to be safe.

crates/lib/examples/parametric_compilation.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/execution_data.rs Outdated Show resolved Hide resolved
crates/lib/src/qpu/result_data.rs Show resolved Hide resolved
crates/lib/src/qpu/result_data.rs Show resolved Hide resolved
crates/python/qcs_sdk/_execution_data.pyi Outdated Show resolved Hide resolved
crates/python/qcs_sdk/_execution_data.pyi Outdated Show resolved Hide resolved
crates/python/src/execution_data.rs Show resolved Hide resolved
crates/python/src/qpu/result_data.rs Outdated Show resolved Hide resolved
@MarquessV
Copy link
Contributor Author

Main reason for requesting changes is that I ran cargo check --all-targets --all-features locally and it surfaced some errors inside of tests. It may also be worth running cargo clippy with the same flags, just to be safe.

Those checks fail for the same reasons on main. There is a manual-tests feature that has been neglected. Related issue here: #218

Copy link
Contributor

@Shadow53 Shadow53 left a comment

Choose a reason for hiding this comment

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

Three things:

  1. Enum variants got turned into tuples in a pyi file
  2. cargo deny check fails, in part due to the addition of numpy
  3. cargo doc and cargo deadlinks --check-intra-doc-links show warnings. Not sure how many were there previously, but worth at least looking at.

crates/python/qcs_sdk/_executable.pyi Show resolved Hide resolved
@MarquessV
Copy link
Contributor Author

I added the numpy license to deny.toml to satisfy cargo deny. The doc warnings and deadlinks are all pre-existing. The latter looks like it has to do with code generated by the pyo3 bindings.

crates/lib/src/qpu/result_data.rs Show resolved Hide resolved
Copy link
Contributor

@Shadow53 Shadow53 left a comment

Choose a reason for hiding this comment

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

LGTM

@MarquessV MarquessV merged commit 7114f71 into main Feb 16, 2023
@MarquessV MarquessV deleted the 215-execution-results-in-common-struct branch February 16, 2023 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

execution results from QVM and QPU should be exposed through a common struct
5 participants