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

Fix required memory checks for QasmSimulator with custom method #763

Merged
merged 6 commits into from
Jun 1, 2020

Conversation

chriseclectic
Copy link
Member

Summary

Fixes issue where memory requirements of simulation were not being checked on the QasmSimulator when using a non-automatic simulation method.

Closes #755

Details and comments

@chriseclectic chriseclectic requested a review from atilag as a code owner May 29, 2020 16:50
@chriseclectic chriseclectic added Changelog: Bugfix Include in the Fixed section of the changelog stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable labels May 29, 2020
@chriseclectic chriseclectic added this to the 0.5.2 milestone May 29, 2020
Copy link
Member

@atilag atilag left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -352,12 +352,8 @@ template <class densmat_t>
size_t State<densmat_t>::required_memory_mb(uint_t num_qubits,
const std::vector<Operations::Op> &ops)
const {
// An n-qubit state vector as 2^n complex doubles
// where each complex double is 16 bytes
(void)ops; // avoid unused variable compiler warning
Copy link
Member

Choose a reason for hiding this comment

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

This is not part of your changes, but seeing this line, it makes clear that we need an override of this method.

Comment on lines +1 to +5
---
fixes:
- |
Fixes issue where memory requirements of simulation were not being checked
on the QasmSimulator when using a non-automatic simulation method.
Copy link
Member

Choose a reason for hiding this comment

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

I thought this was automatically generated when creating the release?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought you still needed these since they have them in Terra, but maybe @mtreinish knows?

Copy link
Member

Choose a reason for hiding this comment

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

Yes this is still needed. The changelog is automated now, but release documentation still needs to be written. The previous changelog was serving double duty as release documentation (frankly quite poorly). See the contributing docs on this: https://github.com/Qiskit/qiskit-aer/blob/master/CONTRIBUTING.md#release-notes (the changelog section above it mentions the changelog autogeneration)

@chriseclectic chriseclectic merged commit f26cb6a into Qiskit:master Jun 1, 2020
chriseclectic added a commit to chriseclectic/qiskit-aer that referenced this pull request Jun 1, 2020
chriseclectic added a commit to chriseclectic/qiskit-aer that referenced this pull request Jun 1, 2020
chriseclectic added a commit to chriseclectic/qiskit-aer that referenced this pull request Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the Fixed section of the changelog stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segmentation Fault during repeated execution of circuit using density_matric_gpu simulator)
3 participants