Hannibalcarthaga commented 3 years ago

These are the preliminary files for the Issue #572 [unitaryhack].

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!
[ ] 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 or black -l 100 strawberryfields. You will need to have the Black code format installed: pip install black.
[ ] 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 run pylint 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: Issue #572 [unitaryhack]

Description of the Change: Added threshold_detection_prob_displacement function.

Benefits:

Possible Drawbacks:

Related GitHub Issues: Errors when running pylint test.

codecov[bot] commented 3 years ago

Codecov Report

Merging #590 (6c872df) into master (6b7b5c5) will decrease coverage by 0.04%. The diff coverage is 33.33%.

:exclamation: Current head 6c872df differs from pull request most recent head f47203a. Consider uploading reports for the commit f47203a to get more accurate results
@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
- Coverage   98.44%   98.40%   -0.05%     
==========================================
Files          76       76              
Lines        8752     8758       +6     
==========================================
+ Hits         8616     8618       +2     
- Misses        136      140       +4     

Impacted Files	Coverage Δ
...rawberryfields/backends/gaussianbackend/backend.py	`95.52% <33.33%> (-2.92%)`	:arrow_down:

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 6b7b5c5...f47203a. Read the comment docs.

nquesada commented 3 years ago

Hi @Hannibalcarthaga : as a first suggestion, I'd like to ask you to start looking into the CodeFactor issues reported by the first bot.

Hannibalcarthaga commented 3 years ago

Hi, @nquesada I have tried to resolve most of the errors from the CodeFactor issues, but some still remain, could you suggest how to resolve them?

Hannibalcarthaga commented 3 years ago

Hi, @nquesada I have imported the functions and committed those changes, but CodeFactor issues are mostly about these Unused imports. Another thing that I am confused about is that 2 errors are undefined variables hbar and det_pattern. Should I add sf.hbar, and the det_pattern definition from thewalrus documentation? Lastly, in the add_mode does the number of modes needs to be changed from n=1 to n>1?

nquesada commented 3 years ago

Hi @Hannibalcarthaga ---- I am trying to understand what your PR achieves. Where have you added the functionality that allows to support for threshold detection of Gaussian states with displacement.

Hannibalcarthaga commented 3 years ago

Hi @Hannibalcarthaga ---- I am trying to understand what your PR achieves. Where have you added the functionality that allows to support for threshold detection of Gaussian states with displacement.

@nquesada I could only add the return command replacing the not implemented error in measure_threshold. Will the functionality be similar to those of hafnian_sample_state and torontonian_sample_state that defines the arguments in the brackets, except in this case for threshold_detection_prob_displacement? Sorry, I am asking many questions, this is my first time contributing on an open source issue and I am not sure how to proceed further.

josh146 commented 3 years ago

Hi @Hannibalcarthaga,

Sorry, I am asking many questions, this is my first time contributing on an open source issue and I am not sure how to proceed further.

No worries at all! This is quite a challenging PR to be working on for a first time contribution (and we recognized that on our side as well; it was even tagged for a 'bounty' during UnitaryHACK, which recently concluded last week). So huge props for attempting it!

For a first time contribution, however, I recommend perhaps looking over at the https://github.com/pennylaneai/pennylane repository, in particular, the issues we have labelled as good first issue?

crazy4pi314 commented 3 years ago

Hey @Hannibalcarthaga 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!

Hannibalcarthaga commented 3 years ago

Hi @nquesada, I have added some small changes regarding the functionality to the backend.py file and made a new commit. May I ask you to check if they are correct or suggest any changes? Thanks.

nquesada commented 3 years ago

Hi @Hannibalcarthaga --- I am having a lot of trouble reviewing your PR. In particular the diff in backend.py makes it look as if you had written from scratch that file, which I am sure is not the case. Without being able to see clearly what your additions were I cannot review it.

co9olguy commented 3 years ago

Hi @Hannibalcarthaga, as @nquesada mentioned above, it is difficult for us to see the changes you made without parsing manually through a 400-line file.

In order to use the nice features of git/github, you'll need to update the file located at strawberryfields/strawberryfields/backends/gaussianbackend/backend.py and check your changes into version control and push here (rather than adding a copy of the file at the base directory, which is what it seems you have done). Then GitHub will be able to show us the actual changes you've made and remove a barrier that is presently preventing us from proper review

Hannibalcarthaga commented 3 years ago

Hey @nquesada, sorry for the inconvenience caused. I had some trouble with Git Credential Manager and couldn't push the changes from local repository. As @co9olguy mentioned I made the commits from GitHub website via file upload which I think caused the large diff. I have resolved those git issues and pushed a new commit Add functions. With this commit two files got automatically uploaded that shouldn't have, but I will resolve that as soon as possible. I am sorry for the inconvenience. Please suggest any changes.

co9olguy commented 3 years ago

Hi @Hannibalcarthaga, I've taken the liberty of deleting those extraneous files from the PR. You'll need to pull and merge these changes into your local branch.

Hannibalcarthaga commented 3 years ago

Hi @Hannibalcarthaga, I've taken the liberty of deleting those extraneous files from the PR. You'll need to pull and merge these changes into your local branch.

Thanks a lot. :innocent:

Hannibalcarthaga commented 3 years ago

Hey @co9olguy can I request a review on the recent commit created a test ? I have made a test with the assert statement as per your suggestion. Thank you :innocent:

co9olguy commented 3 years ago

Hi @Hannibalcarthaga, sorry for delayed response, I was on vacation and getting caught up now.

I regret that I have to share the following decision: after consulting with the team, we don't believe that this PR is on a path to successful acceptance without requiring significant additional guidance from the developers (which regretably, we will not be able to commit to, so long after unitaryHACK)

I do want to think you for your interest in our software and your work so far to attempt this PR :muscle:. If you remain interested in the library, there might be some more beginner-friendly contributions in the future that could be better "good first issues" to tackle :slightly_smiling_face:

Hannibalcarthaga commented 3 years ago

Hi @Hannibalcarthaga, sorry for delayed response, I was on vacation and getting caught up now.

I regret that I have to share the following decision: after consulting with the team, we don't believe that this PR is on a path to successful acceptance without requiring significant additional guidance from the developers (which regretably, we will not be able to commit to, so long after unitaryHACK)

I do want to think you for your interest in our software and your work so far to attempt this PR . If you remain interested in the library, there might be some more beginner-friendly contributions in the future that could be better "good first issues" to tackle

Hey @co9olguy, you and others of the Xanadu team have helped me a lot during the past two months to tackle this PR and I am grateful for it. I want to thank you for giving me this opportunity and next time I will be sure to select a good first issue. Thank you. :innocent:

co9olguy commented 3 years ago

Thanks @Hannibalcarthaga! :slightly_smiling_face: I hope you were able to learn some valuable new skills in the process, even if the PR attempt wasn't successful

XanaduAI / strawberryfields

[unitaryhack]Add support for threshold detection of Gaussian states with displacement #590

Before submitting

Codecov Report