-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix #43: Validate and add docs for Gaussian states containing symbolic variables #44
Conversation
I forgot to add the Symbolics.jl to test REPL tests :) Testing Running tests...
Starting tests with 1 threads out of `Sys.CPU_THREADS = 4`...
[ Info: SetupBuildDirectory: setting up build directory.
[ Info: Doctest: running doctests.
[ Info: Skipped ExpandTemplates step (doctest only).
[ Info: Skipped CrossReferences step (doctest only).
[ Info: Skipped CheckDocument step (doctest only).
[ Info: Skipped Populate step (doctest only).
[ Info: Skipped RenderDocument step (doctest only).
Test Summary: | Pass Total Time
Package | 264 264 2m57.4s
Testing Gabs tests passed We have to add "Missings" that is in the manifest to |
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
The PR is ready for review :), Please let me know your thoughts, Thank you! |
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.
Hi Feroz, thanks for this contribution! I left some minor comments on your proposed changes, but besides that it looks good to me. Could you also please do the following:
- Add tests for
thermalstate
. - Add tests for tensor products of Gaussian states containing symbolic variables.
- Not in this PR, but in the future, validations should be done with Gaussian unitaries, channels, and application products onto Gaussian states. If you don't plan to submit a PR related to this anytime soon, then could you add a comment in Validate and add docs for Gaussian objects containing symbolic variables #43 that references this PR and states that more tests need to be done for Gaussian operations?
Co-authored-by: Andrew Kille <[email protected]>
Co-authored-by: Andrew Kille <[email protected]>
Co-authored-by: Andrew Kille <[email protected]>
The CI errors occurred are due doctest error caused by:
I think we can remove Something like: Use Latexify to render The tests pass now :)
|
Hi, Andrew, The Line 91 in e5d5398
Similar to other states, we can use Line 247 in e5d5398
or Line 166 in e5d5398
Following this fix, we can do julia> using Gabs
julia> using Symbolics
julia> nmodes = rand(1:5)
4
julia> qpairbasis = QuadPairBasis(nmodes)
QuadPairBasis(4)
julia> qblockbasis = QuadBlockBasis(nmodes)
QuadBlockBasis(4)
julia> @variables n
1-element Vector{Num}:
n
julia> thermalstate(qpairbasis, n)
GaussianState for 4 modes.
symplectic basis: QuadPairBasis
mean: 8-element Vector{Num}:
0
0
0
0
0
0
0
0
covariance: 8×8 Matrix{Num}:
0.5 + n 0 0 0 0 0 0 0
0 0.5 + n 0 0 0 0 0 0
0 0 0.5 + n 0 0 0 0 0
0 0 0 0.5 + n 0 0 0 0
0 0 0 0 0.5 + n 0 0 0
0 0 0 0 0 0.5 + n 0 0
0 0 0 0 0 0 0.5 + n 0
0 0 0 0 0 0 0 0.5 + n Although, we need to make sure that adding the symbolic support does not break anything. |
Thank you for your comments! I have incorporated all of your comments :) I have tweaked the thermal states methods so they work with symbolic states as well and tested the symbolic thermal states with both variables and arrays of variables so that it does not break with the any tests that use Test summary :)
|
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.
Hi Feroz, thanks for all of these additions! To deal with the Manifest.toml file (which shouldn't be in this repository and is a result of only me working on it so far 😄 ), can you add a .gitignore file with the Manifest file in it? Then we can merge this!
Co-authored-by: Andrew Kille <[email protected]>
Co-authored-by: Andrew Kille <[email protected]>
Co-authored-by: Andrew Kille <[email protected]>
Add manifest in .gitignore
Thank you for your comments! I have incorporated all of your suggestions! Test summary :)
Edit: Sorry, I didn't understood properly your request about |
Merged! Thank you for the contribution! |
This PR aims to resolve #43 by validating via tests and adding docs for Gaussian objects containing symbolic variables.
Symbolics.jl
is added to manifest. When I added the packages, the manifest got updated on it's own, so I didn't directly edit it per se, it got automated by machine. Please let me know should I revert the changes to manifest? I am using1.11.3
so it changed Julia version to1.11.3
from1.11.1
I think we have to check all the states to validate which states actually support symbolic operations. So, far I have tested the following states:
squeezedstate
coherentstate
Please let me know your thoughts! Thanks you!