Closed federico0112 closed 3 years ago
Hi @federico0112 : as a first suggestion, I'd like to ask you to start looking into the CodeFactor
issues reported by the first bot.
@federico0112 : it looks like you are almost there in terms of linting. You only need two three more docstrings and a whitespace issue.
Merging #591 (6c2f3a6) into master (07c6f95) will increase coverage by
0.02%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #591 +/- ##
==========================================
+ Coverage 98.42% 98.44% +0.02%
==========================================
Files 75 76 +1
Lines 8608 8752 +144
==========================================
+ Hits 8472 8616 +144
Misses 136 136
Impacted Files | Coverage Δ | |
---|---|---|
strawberryfields/compilers/__init__.py | 100.00% <100.00%> (ø) |
|
strawberryfields/compilers/gaussian_merge.py | 100.00% <100.00%> (ø) |
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 07c6f95...6c2f3a6. Read the comment docs.
@federico0112 : can you pass black -l 100
in these two files:
strawberryfields/compilers/__init__.py
strawberryfields/compilers/gaussian_merge.py
@federico0112 : for what I can see you have not added new tests. A trivial tests this compiler should have is that it should pass all the tests the Gaussian compiler passes. Then you should create a new test file in tests/frontend/compilers
that tests the new functionality.
Can this test be the one you gave me? Would that be enough or do I need more test cases
That is a potential test, but more are needed. For those I suggest you for example try to compile the program showed in the issue [#574]. Keep in mind that you will want to use small parameters for all the gates that generate photons so that you don't need gigantic photon number cutoff. Moreover, you will want to modify tests for gaussian_unitary
so that your now compiler also passes. For example this test you should be able to repurpose for your compiler by something like
@pytest.mark.parametrize("depth", [1, 3, 6])
@pytest.mark.parametrize("width", [5, 10, 15])
@pytest.mark.parametrize("compiler", ["gaussian_unitary", "gaussian_merge"])
def test_gaussian_program(depth, width, compiler):
"""Tests that a circuit and its compiled version produce the same Gaussian state"""
eng = sf.LocalEngine(backend="gaussian")
eng1 = sf.LocalEngine(backend="gaussian")
circuit = sf.Program(width)
with circuit.context as q:
for _ in range(depth):
U, s, V, alphas = random_params(width, 2.0 / depth, 1.0)
ops.Interferometer(U) | q
for i in range(width):
ops.Sgate(s[i]) | q[i]
ops.Interferometer(V) | q
for i in range(width):
ops.Dgate(np.abs(alphas[i]), np.angle(alphas[i])) | q[i]
compiled_circuit = circuit.compile(compiler=compiler)
cv = eng.run(circuit).state.cov()
mean = eng.run(circuit).state.means()
cv1 = eng1.run(compiled_circuit).state.cov()
mean1 = eng1.run(compiled_circuit).state.means()
assert np.allclose(cv, cv1)
assert np.allclose(mean, mean1)
Ah I see, thats interesting I've never done test coverage in Python. Ill work on getting the test coverage up. Is this required for the bounty? I am going to be busy all tonight and I don't know if I'll be able to finish it by midnight tonight. It's okay if it is required, just curious.
HI @federico0112 : Great progress so far. There are still a few lines of gaussian_merge
that are not being covered by the tests: https://app.codecov.io/gh/XanaduAI/strawberryfields/compare/591/diff#diff-c3RyYXdiZXJyeWZpZWxkcy9jb21waWxlcnMvZ2F1c3NpYW5fbWVyZ2UucHk=
It seems like most of them are related to displacements.
I made a change to the unit test. It should increase the code coverage
Hi @federico0112 : there still some part of the compiler not covered by the tests (https://app.codecov.io/gh/XanaduAI/strawberryfields/compare/591/diff#diff-c3RyYXdiZXJyeWZpZWxkcy9jb21waWxlcnMvZ2F1c3NpYW5fbWVyZ2UucHk=). You might need to design some specific tests to cover those edge cases.
I just realized that I left out a function that was not being used anymore (figured out a better way to achieve the same thing). Thats why it wasn't covered by the unit tests.
Great work so far @federico0112! I just had a look at the coverage (link here: https://app.codecov.io/gh/XanaduAI/strawberryfields/compare/591/tree/strawberryfields/compilers/gaussian_merge.py) and it looks like there are a couple of lines still not being tested:
Do you think you could modify existing tests where it makes sense to ensure that these parts of the logic are being tested?
Looks like all the checks are now passing, awesome work @federico0112. However, don't forget to also update the .github/CHANGELOG.md
! You will need to add three things:
Would I add it under the development release section?
@federico0112 : yes
Finished updating CHANGLOG.md let me know im missing anything
Hey @federico0112 and the PennyLane team! I wanted to let folks know I am extending the unitaryHACK deadline till the end of this week for PRs that were already in progress to give some time to iterate with maintainers. Let me know if you have questions, or if I can be of help in getting this issue closed!
Thanks @crazy4pi314!
This is a fantastic addition to Strawberry Fields, thanks for all the hard work @federico0112 .
Dear @crazy4pi314 --- The fantastic addition by @federico0112 is now merged into SF. It was a lot of fun being part of the unitaryhack this year. Looking forward to the next one!
Adds hybrid Gaussian Merge compiler that merges gaussian operations into Gaussian Tranform operations in a program with non-Gaussian and Gaussian operations.
Before submitting
Please complete the following checklist when submitting a PR:
[x] All new features must include a unit test. If you've fixed a bug or added code that should be tested, add a test to the test directory!
[x] All new functions and code must be clearly commented and documented. If you do make documentation changes, make sure that the docs build and render correctly by running
make docs
.[x] Ensure that the test suite passes, by running
make test
.[x] Ensure that code is properly formatted, by running
make format
orblack -l 100 strawberryfields
. You will need to have the Black code format installed:pip install black
.[x] Add a new entry to the
.github/CHANGELOG.md
file, summarizing the change, and including a link back to the PR.[x] The Strawberry Fields source code conforms to PEP8 standards. We check all of our code against Pylint. To lint modified files, simply
pip install pylint
, and then runpylint strawberryfields/path/to/file.py
.When all the above are checked, delete everything above the dashed line and fill in the pull request template.
Context: https://github.com/XanaduAI/strawberryfields/issues/574
Description of the Change: Adds hybrid Gaussian Merge compiler that merges gaussian operations into Gaussian Tranform operations in a program with non-Gaussian and Gaussian operations.
Benefits: Can potantially reduce calculation overhead by reducing the number of gaussian operations in a circuit
Possible Drawbacks: Circuits with displacement and squeezing gates require precise cutoff values, determining the correct cutoff and displacement/squeezing parameters has to be done manually.
Related GitHub Issues: https://github.com/XanaduAI/strawberryfields/issues/574