Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

Integrate changes of noise aware simulation #72

Merged
merged 106 commits into from
Jul 5, 2022
Merged

Conversation

33Gjl1Xe
Copy link
Collaborator

@33Gjl1Xe 33Gjl1Xe commented Jun 10, 2022

This PR adds the latest changes of the noise aware quantum circuit simulation to the DDPackage.
More precisely, this includes functionality of the stochastic noise aware quantum circuit simulator (mainly caching of operations) and functionality of the deterministic noise aware quantum circuit simulator (a new node type, multiplication and addition for the new node type and caching of noise operations.).

@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #72 (555ed6c) into main (37d974f) will decrease coverage by 0.0%.
The diff coverage is 94.9%.

@@           Coverage Diff           @@
##            main     #72     +/-   ##
=======================================
- Coverage   95.8%   95.7%   -0.1%     
=======================================
  Files         19      20      +1     
  Lines       3462    3800    +338     
=======================================
+ Hits        3318    3640    +322     
- Misses       144     160     +16     
Impacted Files Coverage Δ
include/dd/Complex.hpp 100.0% <ø> (ø)
include/dd/Package.hpp 91.9% <89.6%> (-0.5%) ⬇️
include/dd/ComplexNumbers.hpp 98.3% <100.0%> (-0.1%) ⬇️
include/dd/ComplexTable.hpp 93.7% <100.0%> (+<0.1%) ⬆️
include/dd/ComputeTable.hpp 92.0% <100.0%> (+0.3%) ⬆️
include/dd/DensityNoiseTable.hpp 100.0% <100.0%> (ø)
include/dd/Edge.hpp 100.0% <100.0%> (ø)
include/dd/Node.hpp 100.0% <100.0%> (ø)
include/dd/StochasticNoiseOperationTable.hpp 100.0% <100.0%> (ø)
include/dd/UniqueTable.hpp 97.2% <100.0%> (+<0.1%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37d974f...555ed6c. Read the comment docs.

Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Thanks again for all the work.
I have a couple of really minor comments. These shouldn't take long to implement at all. Most are just discussions about proper naming and proper types.

A major concern is, that the functionality of the new class (at least at the moment) depends on the enums from the QFR. This is somewhat ugly (and also one of the main reasons the package does not compile at the moment).
I would argue that it would be perfectly fine to take this class and move it to the include/dd part of the QFR.
Then you can even define the StochasticNoisePackage there and provide it as a default argument for the newly created class.
Seems to me as if this would solve all the problems.
It's probably for the best to quickly work through all the comments here, commit them and resolve the comments. Then take the class and move it to the QFR.

include/dd/Package.hpp Outdated Show resolved Hide resolved
include/dd/NoiseFunctionality.hpp Outdated Show resolved Hide resolved
include/dd/NoiseFunctionality.hpp Outdated Show resolved Hide resolved
include/dd/NoiseFunctionality.hpp Outdated Show resolved Hide resolved
include/dd/NoiseFunctionality.hpp Outdated Show resolved Hide resolved
include/dd/NoiseFunctionality.hpp Outdated Show resolved Hide resolved
include/dd/NoiseFunctionality.hpp Outdated Show resolved Hide resolved
include/dd/NoiseFunctionality.hpp Outdated Show resolved Hide resolved
include/dd/NoiseFunctionality.hpp Outdated Show resolved Hide resolved
include/dd/NoiseFunctionality.hpp Outdated Show resolved Hide resolved
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

I believe we are almost done with this. Just a couple more remarks and requests here and there. Should be nothing major.

One thing GitHub wouldn't let me annotate inline:
The coverage seems to indicate that density matrices are never added? can the corresponding compute table be removed entirely?

Also, please give this PR a proper name and add a description.

include/dd/Edge.hpp Outdated Show resolved Hide resolved
include/dd/Package.hpp Outdated Show resolved Hide resolved
include/dd/Package.hpp Outdated Show resolved Hide resolved
include/dd/Package.hpp Outdated Show resolved Hide resolved
include/dd/Package.hpp Outdated Show resolved Hide resolved
test/test_package.cpp Show resolved Hide resolved
test/test_package.cpp Outdated Show resolved Hide resolved
test/test_package.cpp Outdated Show resolved Hide resolved
test/test_package.cpp Outdated Show resolved Hide resolved
test/test_package.cpp Outdated Show resolved Hide resolved
@33Gjl1Xe 33Gjl1Xe changed the title Merging latest changes of the noise aware simulator Integrate changes of noise aware simulation Jun 30, 2022
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

I have one tiny little remark which needs to be fixed, but after that, I would gladly merge this PR 🥳

include/dd/Package.hpp Outdated Show resolved Hide resolved
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

LGTM and ready to merge 🥳 I'll merge once the other PRs are fully reviewed.

@33Gjl1Xe
Copy link
Collaborator Author

33Gjl1Xe commented Jul 5, 2022

Happy to hear. It took some time and I am happy with the result. Thank you for your (quick) feedback and suggestions. @burgholzer

@burgholzer burgholzer merged commit 7f6bf13 into main Jul 5, 2022
@burgholzer burgholzer deleted the noiseComputeTable branch July 5, 2022 14:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants