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

Daisyrainsmith/commutation2 #417

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

daisyrainsmith
Copy link

Adding commutation logic
Rewrite of PR # 386

daisyrainsmith and others added 16 commits June 8, 2021 19:06
Update to add commutation rules to the optimizer engine.
1. Update _optimizer.py and _optimizer_test.py
- apply_commutation Boolean – tells LocalOptimizer whether to consider commutation rules
- Rearranged existing code to streamline and improve readability.
- Restructuring of optimizer to check if the next command is commutable with the current command, or if it may be part of a commutable circuit.
2. Update _basics.py and _basics_test.py
- Added attributes to do with commutation rules.
3. Update _command.py and _command_test.py
- Added commutation attributes and overlap function to streamline optimizer code.
4. Update _gates.py and _gates_test.py
- Added attributes to do with commutation rules.
- Updated Rxx, Ryy, Rzz to have interchangeable qubit indices.
5. Update _metagates.py
- Added commutable rule.
6. Addition of _relative_command.py and _relative_command_test.py
- Dummy command which can be used without an engine, used to define commutation rules.
7. Update restricted_gate_set.py
- By default set apply_commutation = True
- Commutability enum
- Commutation rules for X with Rx, Ph and SqrtX, plus tests in _optimize_test, _command_test and _gates_test
- Added more commutable gates to the _commutable_gates list for gates: X, Y, Z, Rx, Ry, Rz, Rxx, Ryy, Rzz, SqrtX, Ph, S, T, R
- Added parameterized unit tests to check these commutation rules are recognized by the LocalOptimizer
- Minor stylistic changes
- I haven’t added tests to check that S, T and SqrtX commute through their commuting partners because as far as I know they don’t cancel/merge with themselves, so there is no way of checking/no need for that functionality.
- Commutability Enum changed to IntEnum.
- Commutable circuit added to Ph and R, and commutable circuit tests in optimize_test parameterised to test R, Rz, Ph.
- Also fix some errors in the tests where Rxx, Ryy and Rzz were
  assumed to be single-qubit gates

- Potential API break for users:
  Rxx(1.0) | qubit1 + qubit2
  was considered valid and now should be replaced with:
  Rxx(1.0) | (qubit1, qubit2)
Adding commutation logic
Rewrite of PR # 386
@Takishima
Copy link
Collaborator

There are a lot of failing tests that you should probably fix before I will review this again.

@daisyrainsmith
Copy link
Author

Hi Takishima,

Could you be more specific? When I run tests on my computer, ops and cengines pass all tests. These are the only folders which I expect to be affected by the new code. There are a lot of tests that fail that I can't see any relation to my new code, such as: libs/math/_gates_math_test and backends/_aqt/_aqt_test. Could you tell me which tests fail for you that you think are related to the new code?

Thanks,

Daisy.

@Takishima
Copy link
Collaborator

Takishima commented Nov 18, 2021

Ok, so here is a small breakup of the CI test failures:

Git issues

Try to run a diff using Git between this branch and the latest develop:

git diff -bw --diff-algorithm=patience develop..HEAD --stat

and then maybe:

git diff -bw --diff-algorithm=patience develop..HEAD

And see if any files appear, beside the ones you know you have modified. Since one of your commit mentions a manual rebase, you might have overlooked something.

CI failures

Static analysis

Link: https://github.com/ProjectQ-Framework/ProjectQ/actions/runs/1445382789

Since the last major code update, ProjectQ is now completely checked using static analysis tools like:

  • clang-tidy
  • black
  • isort
  • flake8
  • pylint

Some of the issues can be fixed very easily on your side (mainly all the issues linked to formatting discrepancies). In order to solve those, simply do the following:

  • Install pre-commit (https://pre-commit.com/)

  • Run pre-commit from within the root directory of your ProjectQ installation:

    pre-commit run --all-files --hook-stage manual
    
  • Commit the changes

For the other checks (ie. mainly linters such as flake8 and pylint) you will need to address the issues one by one. The error messages here should be pretty explanatory.

My suggestions for errors due to too many statements or too many branches (pylint), try to remove them by restructuring the code if you are close to the limit, but. if it is not feasible or not reasonable, you may add a comment to tell the tool to ignore the warning for the particular case.

Unit tests

Link: https://github.com/ProjectQ-Framework/ProjectQ/actions/runs/1445382791

In these tests, some of the errors are indeed quite puzzling and I did not have the time to investigate them in much details over the last few days, but I'll try to dive deeper next week if I find some time.

For sure, the errors about IndexErrors located in _command.py at line 222 looks very suspicious and you may ignore them for now.

Some others are probably linked to the fact that the new optimizer is enabling commutation by default and so therefore some of the assumptions made when writing tests are not satisfied anymore. This is most likely the case of the test_chooser_Ry_reducer test.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Takishima
❌ daisyrainsmith
You have signed the CLA already but the status is still pending? Let us recheck it.

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

3 participants