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

the Bristolian #316

Merged
merged 21 commits into from
Feb 3, 2022
Merged

the Bristolian #316

merged 21 commits into from
Feb 3, 2022

Conversation

jakeffbulmer
Copy link
Contributor

@jakeffbulmer jakeffbulmer commented Jan 11, 2022

Context:
matrix functions for calculating threshold detector statistics of Fock states interfering in linear optical interferometers.

Description of the Change:
New functions, the "Bristolian" brs and the "Unitary Bristolian" ubrs are added to _permanent.py and imported in __init__.py.

Benefits:
Allows for calculation of threshold detector statistics in experiments where Fock states are interfered in linear optical interferometers.

There are also new functions in _permanent.py called fock_threshold_prob and fock_prob which make it easy to get the probabilities of Fock state interference in linear optics. These latter functions are not imported in the init file, but maybe they could be useful elsewhere?

Possible Drawbacks:
What could possibly go wrong?

Related GitHub Issues:

@codecov
Copy link

codecov bot commented Jan 11, 2022

Codecov Report

Merging #316 (6a7c1a6) into master (4ae3408) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #316   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         1575      1603   +28     
=========================================
+ Hits          1575      1603   +28     
Impacted Files Coverage Δ
thewalrus/__init__.py 100.00% <100.00%> (ø)
thewalrus/_permanent.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 4ae3408...6a7c1a6. Read the comment docs.

@jakeffbulmer jakeffbulmer marked this pull request as ready for review January 12, 2022 17:59
@jakeffbulmer jakeffbulmer requested a review from nquesada January 12, 2022 17:59
thewalrus/_permanent.py Outdated Show resolved Hide resolved
@nquesada
Copy link
Collaborator

Almost there @jakeffbulmer : you just need a test for the branching here:

image

You can likely parametrize one of the test you wrote...

@@ -118,7 +118,7 @@
interferometer,
grad_hermite_multidimensional,
)
from ._permanent import perm, permanent_repeated
from ._permanent import perm, permanent_repeated, brs, ubrs
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jakeffbulmer : you probably want to add brs and ubrs into the __all__ list in line 132

Copy link
Collaborator

Choose a reason for hiding this comment

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

and also into the autosummary in line 78. This is so that they are included when the documentation is generated.

Returns:
float: probability of Fock state, n, scattering to m, through an interferometer, U
"""
assert sum(n) == sum(m)
Copy link
Collaborator

Choose a reason for hiding this comment

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

since this is not a jitted function, it would be better if you RaiseValueError instead of using assert. You will need to also add a test checking that the exception is raised correctly.

Comment on lines 289 to 291
assert len(n) == T.shape[1]
assert len(d) == T.shape[0]
assert T.shape[0] <= T.shape[1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above. RaiseValueError is preferred.

Comment on lines +171 to +172


Copy link
Collaborator

Choose a reason for hiding this comment

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

These tests are really great. Fantastic work @jakeffbulmer !

Copy link
Collaborator

@nquesada nquesada left a comment

Choose a reason for hiding this comment

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

Left some comments about inclusion of functions into init and autosummary and not using asserts inside non-jitted functions.

from numba import jit, prange

from scipy.special import factorial
from scipy.linalg import sqrtm
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems like you don't need sqrtm anymore
image

thewalrus/_permanent.py Outdated Show resolved Hide resolved
jakeffbulmer and others added 3 commits January 19, 2022 20:04
Co-authored-by: Nicolas Quesada <991946+nquesada@users.noreply.github.com>
Copy link
Contributor

@sduquemesa sduquemesa left a comment

Choose a reason for hiding this comment

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

Looking great!

@nquesada
Copy link
Collaborator

nquesada commented Feb 3, 2022

Thanks @sduquemesa

@nquesada nquesada merged commit c96b9da into master Feb 3, 2022
@nquesada nquesada deleted the bristolian branch February 3, 2022 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants