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

Updated Sampler #56

Merged
merged 13 commits into from
Nov 6, 2023
Merged

Updated Sampler #56

merged 13 commits into from
Nov 6, 2023

Conversation

ihincks
Copy link
Contributor

@ihincks ihincks commented Oct 31, 2023

No description provided.

0017-sampler-interface.md Outdated Show resolved Hide resolved
Co-authored-by: Jim Garrison <jim@garrison.cc>
Copy link
Member

@ajavadia ajavadia left a comment

Choose a reason for hiding this comment

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

overall direction and philosophy looks good to me

0017-sampler-interface.md Show resolved Hide resolved
0017-sampler-interface.md Show resolved Hide resolved
0017-sampler-interface.md Show resolved Hide resolved
0017-sampler-interface.md Show resolved Hide resolved
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks for this, Ian. I think there's a few things in here that treat IBM-specific extensions as if they are part of the backend.run general interface, which might want removing / reducing.

On a larger note, I'm wondering how / if we can ensure that Sampler (and to a degree, Estimator) are expandable interfaces, potentially with some form of version negotiation, so we don't end up needing to make a SamplerV3 in the future that might return different types. This maybe is a concern too far and too late at this stage, though.

0017-sampler-interface.md Show resolved Hide resolved
0017-sampler-interface.md Show resolved Hide resolved
0017-sampler-interface.md Outdated Show resolved Hide resolved
0017-sampler-interface.md Show resolved Hide resolved
@blakejohnson blakejohnson added the RFC proposal New RFC is proposed label Oct 31, 2023
@jlapeyre
Copy link
Contributor

jlapeyre commented Nov 2, 2023

My understanding is that primitives are meant to be universal interfaces. But references to other "tooling" above #56 (comment) make this less clear to me. The particular concern I have is that sampling a large number of shots from a modest number of qubits using a simulator might be inefficient to impossible without returning results in an associative array. Is there a way to accommodate this situation within this proposal? Or would that require completely separate tooling?

@blakejohnson
Copy link
Contributor

@jlapeyre that comment is in reference to streaming interfaces, which are out of scope of this RFC.

The particular concern I have is that sampling a large number of shots from a modest number of qubits using a simulator might be inefficient to impossible without returning results in an associative array. Is there a way to accommodate this situation within this proposal?

It is no more difficult than handling the same number of shots from hardware.

@jlapeyre
Copy link
Contributor

jlapeyre commented Nov 2, 2023

It is no more difficult than handling the same number of shots from hardware.

Yes, in principle. I used a simulator as an example because it's easy to quickly generate with a simulator more shots than you can store. Anyone currently doing experiments like this won't be able to use this interface.

Now that I think about it, if I understand you correctly, I agree that rather than special casing accumulating shots in a hash map, it's better handled as part of a more general streaming or "online" tool that can handle custom filters. And I see it's been suggested that this is out of scope: #56 (comment)

I'd consider making the streaming interface the fundamental one and provide a collect function to write to an array. Then build something like this proposal on top of that. But it looks like this is too late for this version.

EDIT: If this proposal is about an API boundary, suppose it doesn't preclude changing the implementation later.

0017-sampler-interface.md Outdated Show resolved Hide resolved
blakejohnson and others added 2 commits November 6, 2023 09:51
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
0017-sampler-interface.md Outdated Show resolved Hide resolved
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
@blakejohnson blakejohnson merged commit 42f3a32 into Qiskit:master Nov 6, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC proposal New RFC is proposed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants