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

Fixed fulltomography #308

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

prateekchawla168
Copy link

Motivation:

I wanted to tomograph and reconstruct a state from the output from a quantum circuit I had implemented in PastaQ, but I kept getting Error: 'empirical_probabilities' is not defined. This led me to explore the project code, and I ended up making the functionality work. I have added my changes as a PR.

Small changes to some files in the project, I have added a list here:

  1. Added .vscode to .gitignore. This is mainly for cleanup. :)
  2. Added entries for Convex, SCS, and MathOptInterface to Manifest. This enables us to unconditionally include fulltomography.jl in the module.
  3. Tiny correction to the README, just ensuring consistency (everything that's part of code, including the ] to go into package mode, is formatted as code instead of regular text)
  4. Moved fulltomography.jl into the module proper, removed the requirement for optional SCS and Convex, and made them as required dependencies.
  5. fulltomography.jl now imports SCS, Convex, and MathOptInterface separately. The earlier import const MOI = SCS.MathOptInterface would throw an error.
  6. The functions in fulltomography.jl had some inconsistent implementations for the maximum likelihood keyword, fixed everything to be consistent.
  7. Modified runtests.jl to use a filter, not an explicit list of tests. This can later be upgraded to broadcast the logical && operation on the array returned from readdir(), but requires >=Julia v1.7.
  8. Modified test_fulltomography.jl to use the new keyword for maximum likelihood.
  9. Modified test_gpu.jl to add Phase damping noise as well, and create a new testset for depolarizing noise. This takes care of the fact that even if eltype is nothing, the output of runcircuit in case of depolarizing noise is a ComplexF64, not a Float64.
  10. Modified test_io.jl to write test output data to a data/ subdirectory in the tests folder, so output of testing can be kept separately, keeping the code directory clean. Currently, this directory is added in the .gitignore, but can be removed in case test outputs are needed here.
  11. Verified that all tests pass (at least with single threaded execution, yet to test multithreaded execution).

@mtfishman
Copy link
Collaborator

mtfishman commented Apr 8, 2024

I would prefer to keep them as optional dependencies instead of direct dependencies. Ideally we would organize the code as a package extension: https://pkgdocs.julialang.org/v1/creating-packages/#Conditional-loading-of-code-in-packages-(Extensions).

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.

None yet

2 participants