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

adding iscss for issue #222 #241

Merged
merged 43 commits into from
Mar 20, 2024
Merged

adding iscss for issue #222 #241

merged 43 commits into from
Mar 20, 2024

Conversation

Fe-r-oz
Copy link
Contributor

@Fe-r-oz Fe-r-oz commented Mar 18, 2024

Given the stabilizer has the knowledge, the isscss function is implemented.

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.02%. Comparing base (3bb4653) to head (c674273).

❗ Current head c674273 differs from pull request most recent head b03ed6a. Consider uploading reports for the commit b03ed6a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
+ Coverage   82.60%   88.02%   +5.41%     
==========================================
  Files          56       42      -14     
  Lines        3749     3324     -425     
==========================================
- Hits         3097     2926     -171     
+ Misses        652      398     -254     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Krastanov Krastanov left a comment

Choose a reason for hiding this comment

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

Thank for starting this! Please read a bit more on multiple dispatch and methods in julia and restructure appropriately (see related comments)

https://docs.julialang.org/en/v1/manual/methods/

src/ecc/ECC.jl Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/ecc/ECC.jl Outdated Show resolved Hide resolved
src/ecc/ECC.jl Outdated Show resolved Hide resolved
@Krastanov Krastanov marked this pull request as draft March 18, 2024 15:32
@Krastanov
Copy link
Member

And this probably needs some tests. E.g. for each code check what iscss says and then check whether their parity check matrix is indeed in CSS form. The check would be expensive, it will have to go through the entire tableau of a specific code instance, but it will be happening only when running the test suite.

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Mar 18, 2024

And this probably needs some tests. E.g. for each code check what iscss says and then check whether their parity check matrix is indeed in CSS form. The check would be expensive, it will have to go through the entire tableau of a specific code instance, but it will be happening only when running the test suite.

I thought this was not needed because the stabilizer has the knowledge as you said. Only, tests are to be remaining
Please have a look at modifications. Only test definition is remaining.

so, now I need to use an algorithm that check whether H matrix is in CSS form? Can the stabilizer knowledge can be used here or it's an expensive method, I need to look for a general algorithm in literature?

@Krastanov
Copy link
Member

I thought this was not needed because the stabilizer has the knowledge as you said. Only, tests are to be remaining.

The point of tests is to check whether such assumptions or internal constants are correct. It is not something that runs when the user is using the library, rather it is for the developers to check whether they have made an error (e.g. like setting iscss(::SomeCode) = true when it should not be).

@Krastanov
Copy link
Member

I need to look for a general algorithm in literature?

It is a simple check, not something that you need to look up in the literature. For each row of a parity check tableau check whether it contains a mix of X and Z. As long as they do not, it is a CSS code.

src/ecc/ECC.jl Outdated Show resolved Hide resolved
@Fe-r-oz Fe-r-oz marked this pull request as ready for review March 19, 2024 01:47
@Fe-r-oz Fe-r-oz marked this pull request as draft March 19, 2024 01:51
@Fe-r-oz Fe-r-oz requested a review from Krastanov March 19, 2024 07:58
Copy link
Member

@Krastanov Krastanov left a comment

Choose a reason for hiding this comment

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

Thank you! This is getting pretty close. I left a few comments.

src/ecc/codes/clevecode.jl Outdated Show resolved Hide resolved
src/ecc/codes/fivequbit.jl Outdated Show resolved Hide resolved
src/ecc/codes/gottesman.jl Outdated Show resolved Hide resolved
test/test_ecc_iscss.jl Outdated Show resolved Hide resolved
test/test_ecc_iscss.jl Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@Krastanov
Copy link
Member

I also see you are making a ton of small commits just as you are working through things.

It helps to do the following:

  • try to make commits that are somewhat selfcontained and tell a clear story of how the work is evolving
  • use the --amend flag to just add to the last commit instead of creating a new commit, if they are not logically separated
  • aim to push in one big batch, not after each commit, because each time you push, fairly expensive automated testing is being run on your code. No need to test code you know is unfinished. Generally, run the tests locally (with the ]test QuantumClifford command) before pushing, to make sure the tests pass for you first

@Krastanov
Copy link
Member

do not worry about the buildkite failure -- it will work again after a few new version registrations get propagated through the ecosystem

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Mar 19, 2024

I also see you are making a ton of small commits just as you are working through things.

Thanks for your comments. I will keep this in mind All the requested changes are done.

@Fe-r-oz Fe-r-oz requested a review from Krastanov March 20, 2024 02:36
test/test_ecc_iscss.jl Outdated Show resolved Hide resolved
test/test_ecc_iscss.jl Outdated Show resolved Hide resolved
@Krastanov
Copy link
Member

There are two rather minor issues, but they are also issues that are simply due to inattentiveness, not to some big conceptual problem. Please make sure you reread your code closely before submitting for reviews -- It is important to build up the habit to detect this type of minor issues on your own.

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Mar 20, 2024

There are two rather minor issues

Minor issues are dealth with

@Krastanov
Copy link
Member

I made a minor rephrasing in the changelog. I will wait for the tests and then merge. Thank you, having this done is very helpful!

@Krastanov Krastanov merged commit 7ff9f39 into QuantumSavory:master Mar 20, 2024
9 of 13 checks passed
@Krastanov Krastanov changed the title adding iscss for issue #212 adding iscss for issue #222 Mar 20, 2024
@Krastanov
Copy link
Member

Be more careful with title and references. This closes issue 222, not issue 212. A bit too late to change the commit message now, so we can just close it manually.

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Mar 20, 2024

I made a minor rephrasing in the changelog. I will wait for the tests and then merge. Thank you, having this done is very helpful!

Thank you as well!

@Fe-r-oz Fe-r-oz deleted the isCSS branch July 17, 2024 17:41
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.

2 participants