Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding probability tests #368
Adding probability tests #368
Changes from 8 commits
61b1d97
4a6f391
fa852ca
29f1eab
ae71f5d
c998899
b40d9c1
c8273b6
b36f5c7
3e789fc
ef8ba84
767d5d5
e6a9c3a
dbfd86b
07faa97
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not a relative import like the other imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does that make a difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this function supposed to be? In the docstring it says
density matrix
but should it sayprobability pattern
? Also I don't really understand why there's an option to project onto LO for this, given we assume the PNRs are bucket detectors, it shouldn't affect how we split the internal modes up, no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have rewritten the doc string, hopefully this clarifies the meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, it's essentially the pnr distribution of the 'principal mode' (or mode of our choosing). Still, I feel it might be more appropriate to call it something like
dm_diag_single_mode
(or something to that effect) to avoid confusion withpnr_prob
, but ultimately I don't feel that strongly about it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, PNR prob does something similar, but the signature of the function is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DavidSPhillips this is a waaay too long test..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it take ages in the
internal_modes
branch?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, it takes ages on my computer, that is why I decided against adding the new functions in this test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's too long? The whole test or the addition of the commented out probabilities bit? The new bit doesn't take much longer for my laptop at all (after correcting for the PNR on mode 0). The whole test takes about 2.5 minutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test. 2.5 minutes is too long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the test is still included so it will still take 2.5 minutes. Adding the commented out lines (but not commented out) wouldn't increase the time much.