-
Notifications
You must be signed in to change notification settings - Fork 50
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
enable Lifted Product Code tests and reintroduce piracy #371
Conversation
Benchmark ResultJudge resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
Architecture: x86_64
Benchmark ResultJudge resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
Architecture: x86_64
Benchmark ResultJudge resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
|
Thanks for letting me know this! I think these tests were missed at some late point when I made mistakes in a merge. For (1), I want to first note that the Whether to drop the For (2), I think the problem is that the "adjoint" or "conjugate" of a group algebra element does not seem to be a standard math notation. But here, it would be convenient to define the "conjugate" of a group algebra element by inversing all of its group elements and keeping the coefficients. This convention is also seen in the literature on lifted product codes. To me, it also looks like a naming problem. To resolve the piracy issue, we may just change the name "adjoint" and make it an internal function. Doing so would also require adjustment of |
Sounds good! I removed the lift piracy (what you did is frequently referred to as a "pun" in the julia ecosystem as you used a function that has a good name to implement a method that does not match how the function is already used). Check the last commit I added -- it simplified the representation code a bit in order to make it possible to directly provide a Hecke function as Thus, I think the only thing left to do is fix the adjoint piracy. I like the suggestion you made for a fix, but when I tried to implement it I got something wrong (I do not have a very good understanding of Nemo and Hecke). Please submit it when you have a chance (it is not urgent though, no need to rush) |
a side note -- I changed one of your nested vcat/hcat statements to |
@Krastanov I have just locally fixed the piracy issue and will soon make a PR for it with some documentation enhancement. (I was previously not aware of the "pun" problem. Thanks for letting me know!) The |
@royess , it seems the tests in your PR were never actually running. Part of them were missing because the dictionary of codes was not set properly (you entered the public function as a key instead of the private type). Part of them were not enabled because they were pasted outside of the testitem definition.
These two issues are now fixed, but that led to actually having errors in the PR, partially due to the fix to the piracy issue that the PR had introduced.
Could you look into this sometime soon? In particular, (1) given how you are using the lift function, maybe we can completely drop the
repr::Function
and just rely on Nemo/Hecke; (2) we need a fix for the adjoint piracy (potentially having this upstreamed).