ProjectQ-Framework / ProjectQ

ProjectQ: An open source software framework for quantum computing
https://projectq.ch
Apache License 2.0
893 stars 276 forks source link

Daisyrainsmith/commutation2 #417

Open daisyrainsmith opened 3 years ago

daisyrainsmith commented 3 years ago

Adding commutation logic Rewrite of PR # 386

Takishima commented 3 years ago

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

daisyrainsmith commented 3 years ago

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 commented 3 years ago

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:

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:

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 commented 2 years ago

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.

:white_check_mark: Takishima
:x: daisyrainsmith
You have signed the CLA already but the status is still pending? Let us recheck it.