Adding pre-commit

[scarrazza]

Adding a basic pre-commit hook and applying black to all files.

[scarrazza]

@stavros11 this is failing with a tsp+tensorflow issue, which looks familiar to me, do you remember if this is related to seeds?

[andrea-pasquale]

Thanks for implementing this.
I have a few comments:

• For now, are you happy with just adding black? Additionally we can include isort to fix the imports order.
• Since this repository is public we can even add precommit to CI. With this we can either: (1) make CI fail if the user doesn't use the precommit hooks and/or (2) let the action automatically fix any issues with this (in this case we'll have an extra commit that follows the user commits).
• I feel like we need to add somewhere in the docs that we are using pre-commit, under Developer guides probably.

[scarrazza]

@andrea-pasquale thanks for the comments.

For now, are you happy with just adding black? Additionally we can include isort to fix the imports order.

yes, but if you try here, you will see that qibo fails, due to circular import, so I think we can do that in another PR.

Since this repository is public we can even add precommit to CI. With this we can either: (1) make CI fail if the user doesn't use the precommit hooks and/or (2) let the action automatically fix any issues with this (in this case we'll have an extra commit that follows the user commits).

yes, I fully agree.

I feel like we need to add somewhere in the docs that we are using pre-commit, under Developer guides probably.

yes, absolutely.

[stavros11]

Thanks for implementing this. I checked locally and it works for me.

• Since this repository is public we can even add precommit to CI. With this we can either: (1) make CI fail if the user doesn't use the precommit hooks and/or (2) let the action automatically fix any issues with this (in this case we'll have an extra commit that follows the user commits).

I also agree with this. I think option (2) is more friendly for external developers, but we may get many additional commits if people push frequently (not sure if this is an issue).

• I feel like we need to add somewhere in the docs that we are using pre-commit, under Developer guides probably.

I agree, we should add a related section in "How to contribute". We could also add the pre-commit libarary in setup.py as a requirement for the [tests] package, which is oriented towards developers.

@stavros11 this is failing with a tsp+tensorflow issue, which looks familiar to me, do you remember if this is related to seeds?

Yes, I was aware of this from other branches and I can get reproduce it locally. An easy fix is to set atol=1e-5 for this test, but before doing so I would like to understand better where this precision issue comes from and why it happens only with tensorflow, because it is a bit high to classify as numerical error. It should not be related to seeds as this test is not random since we removed the measurements and we are checking the state vector directly.

[andrea-pasquale]

I've fixed the documentation and I've also added the pre-commit action.
@scarrazza I don't know if it will be enabled when we open another PR or if you as admin will need to install the Github action. I tried it to install the action but I think that I don't have the permissions.

[scarrazza]

Thanks, I have granted permission, please try again now.

[andrea-pasquale]

Now the action is there. I believe that it was failing because there were some conflicts.
If you want I can open a dummy PR and perform some checks.
EDIT: Do we actually want it to fail if there are conflicts or should we maybe remove this option?

[scarrazza]

Sure, please go ahead, and indeed conflicts should not fail.

[stavros11]

Thank you for all the updates and tests @andrea-pasquale. I think this can be merged.

[stavros11]

Do you mean

Suggested change

	- **Pre commit**: We use pre-commit to enforce automatation and to format the code.
	- **Pre commit**: We use pre-commit to enforce automation and to format the code.

?

Perhaps we could also say that this is enforced automatically for all PRs.

[andrea-pasquale]

Thanks for catching this.
Yes that's what I meant :)
Now that we've enabled the pre-commit action I should probably update this line.

[scarrazza]

@stavros11 please let me know when you are happy with the atol increase so we can merge this.

[codecov]

Codecov Report

Merging #628 (40802bd) into master (1af0342) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #628   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           80        80           
  Lines        10342     10380   +38     
=========================================
+ Hits         10342     10380   +38     

Flag	Coverage Δ
unittests	100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/qibo/__init__.py	100.00% <ø> (ø)
src/qibo/backends/abstract.py	100.00% <ø> (ø)
src/qibo/gates/special.py	100.00% <ø> (ø)
src/qibo/hamiltonians/__init__.py	100.00% <ø> (ø)
src/qibo/models/__init__.py	100.00% <ø> (ø)
src/qibo/tests/test_dill.py	100.00% <ø> (ø)
src/qibo/tests/test_gates_special.py	100.00% <ø> (ø)
src/qibo/tests/test_models_circuit_qasm.py	100.00% <ø> (ø)
...rc/qibo/tests/test_models_distcircuit_execution.py	100.00% <ø> (ø)
src/qibo/tests/test_models_hep.py	100.00% <ø> (ø)
... and 106 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.