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

Fixing threshold clicks #136

Merged
merged 16 commits into from
Feb 27, 2020
Merged

Fixing threshold clicks #136

merged 16 commits into from
Feb 27, 2020

Conversation

nquesada
Copy link
Collaborator

@nquesada nquesada commented Feb 26, 2020

This PR adds the functions that allow to find the scaling factor of an adjacency matrix so that it has given mean click number when sampled with threshold detectors, addressing issue #100 .

@nquesada
Copy link
Collaborator Author

We can now add the possibility of doing torontonian sampling fixing the mean number of clicks, instead of the mean photon number which we took as a proxy for the former.
We can either:

  1. Modify torontonian_sample_graph(A, n_mean) so that its second argument is the mean number of clicks.
  2. Make a new function.

I am in favour of option 1 but it would require some reworking in sf apps. Any thoughts @josh146 @trbromley ?

@codecov
Copy link

codecov bot commented Feb 26, 2020

Codecov Report

Merging #136 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   97.61%   97.68%   +0.06%     
==========================================
  Files          12       12              
  Lines         839      864      +25     
==========================================
+ Hits          819      844      +25     
  Misses         20       20
Impacted Files Coverage Δ
thewalrus/quantum.py 100% <100%> (ø) ⬆️

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 4ad7a25...ee4e479. Read the comment docs.

Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Just left a couple of quick questions :)

thewalrus/quantum.py Show resolved Hide resolved
thewalrus/quantum.py Show resolved Hide resolved
thewalrus/quantum.py Show resolved Hide resolved
thewalrus/quantum.py Outdated Show resolved Hide resolved
thewalrus/quantum.py Outdated Show resolved Hide resolved
thewalrus/quantum.py Outdated Show resolved Hide resolved
Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @nquesada - would be cool to have this feature and use it in the apps layer.

In terms of how this fits in to SF and the apps layer, I had more of a look just now.
The apps layer has this in sample.sample():

    mean_photon_per_mode = n_mean / float(nodes)

    # pylint: disable=expression-not-assigned,pointless-statement
    with p.context as q:
        sf.ops.GraphEmbed(A, mean_photon_per_mode=mean_photon_per_mode) | q

        if loss:
            for _q in q:
                sf.ops.LossChannel(1 - loss) | _q

        if threshold:
            sf.ops.MeasureThreshold() | q
        else:
            sf.ops.MeasureFock() | q

That is, it uses SF's GraphEmbed rather than directly calling torontonian_sample_graph.

I think it might be good to update torontonian_sample_graph separately, but for users to have access in SF and apps, we'd need to update decompositions.graph_embed(). In there, it would be easy to have find_scaling_adjacency_matrix_torontonian as well as the find_scaling_adjacency_matrix which is currently there, depending on whether we want to set n_mean or n_clicks. In terms of the user interface, how about:

  • User can input mean_photon_per_mode, and we rescale to have that mean photon number.
    OR
  • User can input mean_click_per_mode, and we rescale to have that number of clicks.

If they input both, they will get an exception. In this way, we don't break any existing SF and apps code, and can do a separate PR to apps.sample.sample() to allow for sampling based on mean click number.

thewalrus/quantum.py Outdated Show resolved Hide resolved
thewalrus/quantum.py Show resolved Hide resolved
thewalrus/quantum.py Outdated Show resolved Hide resolved
thewalrus/quantum.py Outdated Show resolved Hide resolved
thewalrus/quantum.py Outdated Show resolved Hide resolved
thewalrus/quantum.py Outdated Show resolved Hide resolved
thewalrus/tests/test_quantum.py Outdated Show resolved Hide resolved
thewalrus/tests/test_quantum.py Show resolved Hide resolved
Co-Authored-By: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-Authored-By: Josh Izaac <josh146@gmail.com>
@codecov-io
Copy link

codecov-io commented Feb 27, 2020

Codecov Report

Merging #136 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   97.61%   97.68%   +0.06%     
==========================================
  Files          12       12              
  Lines         839      864      +25     
==========================================
+ Hits          819      844      +25     
  Misses         20       20
Impacted Files Coverage Δ
thewalrus/quantum.py 100% <100%> (ø) ⬆️

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 4ad7a25...48cd2b2. Read the comment docs.

nquesada and others added 3 commits February 27, 2020 10:22
Co-Authored-By: Josh Izaac <josh146@gmail.com>
Co-Authored-By: Josh Izaac <josh146@gmail.com>
Co-Authored-By: Josh Izaac <josh146@gmail.com>
@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

Merging #136 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   97.61%   97.68%   +0.06%     
==========================================
  Files          12       12              
  Lines         839      864      +25     
==========================================
+ Hits          819      844      +25     
  Misses         20       20
Impacted Files Coverage Δ
thewalrus/quantum.py 100% <100%> (ø) ⬆️

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 4ad7a25...700f214. Read the comment docs.

@nquesada
Copy link
Collaborator Author

@trbromley what you propose sounds good. It also implies that no more changes are needed from this side at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants