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

Unset noise after noise tests #635

Merged
merged 2 commits into from
Sep 12, 2023
Merged

Conversation

bmhowe23
Copy link
Collaborator

@bmhowe23 bmhowe23 commented Sep 9, 2023

This helps fix issue #609 by unsetting noise models at the end of noise tests. Without this change, the prior test's noise model could be used in subsequent tests, which is undesirable.

(Edited later) The following information is no longer applicable but is left for posterity...

In addition, a new test was added to show another related issue existed if the noise model was used outside of its original scope (as the API permits).

The resolution for that is to convert the noise model to shared_ptr instead of raw pointers. It would've been preferable to use unique_ptr, but Python bindings do not allow unique_ptr's to be used as function arguments, and we need the ability pass them into the C++ engine via functions, so shared_ptr was chosen.

@github-actions
Copy link

github-actions bot commented Sep 9, 2023

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Sep 9, 2023
@amccaskey
Copy link
Collaborator

amccaskey commented Sep 9, 2023

Thanks for the fixes here, Ben. I would like to try to find a different approach, as I do not want to make it a requirement that users instantiate a shared_ptr at the language level. They should be free to do so, but we should not enforce it. As for the Python unique_ptr issue, make_kernel() returns a unique_ptr and we pass this instance to other API functions that are bound to Python. In this case I think we just need to be careful about passing by reference vs value in the pybind bindings.

I think there are other approaches that we can take to address the issues we've seen in the runtime. Happy to meet this week to discuss those approaches. One approach may be an update to the noise model destructor to call unset_noise.

@bmhowe23
Copy link
Collaborator Author

bmhowe23 commented Sep 9, 2023

Thanks for the feedback. I'm definitely open to different approaches and was indeed questioning if this was the best path forward. Do you think 447b7ce is a valid use case that should be supported? Maybe the answer is 'no', and if so, maybe the smart pointer portion of these changes aren't needed.

@github-actions
Copy link

github-actions bot commented Sep 9, 2023

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Sep 9, 2023
Issue discovered while investigating Issue NVIDIA#609
@bmhowe23 bmhowe23 changed the title Unset noise after noise tests and convert noise models to smart pointers Unset noise after noise tests Sep 12, 2023
@bmhowe23
Copy link
Collaborator Author

After further discussion with @amccaskey, we decided to remove the smart pointer portion of this PR and instead just a) make it clear when the user must call unset_noise(), and b) update our tests to be compliant with that guidance.

@bmhowe23 bmhowe23 marked this pull request as ready for review September 12, 2023 14:06
@github-actions
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Sep 12, 2023
@github-actions
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Sep 12, 2023
@bmhowe23 bmhowe23 merged commit 93382d6 into NVIDIA:main Sep 12, 2023
@bmhowe23 bmhowe23 deleted the ch-unset-noise branch September 12, 2023 17:29
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2023
@bmhowe23 bmhowe23 added this to the release 0.4.1 milestone Sep 25, 2023
@bettinaheim bettinaheim added the maintenance Work items to update and improve the code base label Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintenance Work items to update and improve the code base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants