Skip to content

Conversation

@Takishima
Copy link
Collaborator

@Takishima Takishima commented Jun 30, 2021

Summary of changes

  • Fix a bug introduced recently in the testing for the phase estimation decomposition
  • Fix a small bug in drawing with matplotlib
  • Update all docstrings to be PEP257 compliant
  • Move some common exception classes to their own dedicated files
  • Add isort to the list of pre-commit hooks and run it on the whole codebase
  • Add flake8-breakpoint, flake8-comprehensions, flake8-eradicate, flake8-mutable to pre-commit configuration file and run on the whole codebase
  • Remove some Python 2.x compatibility code (mostly constructor calls and trivial definitions of __ne__)
  • Update the documentation code in docs/

Reviewing

There are a lot of changes in the code that are only the result of running isort, running a few more flake8 plugins, updating the docstrings to make them PEP257 compliant as well as replacing some Python 2 code like:

  • changing constructor calls from the old style (BasicEngine.__init__(self) to the new style (super().__init__())
  • removing the trivial definitions of __ne__ methods since Python 3 guarantees that __ne__ calls __eq__ if that one is defined.

Here is a list of files that have some changes in them that are not immediately related to aforementioned issue to make reviewing this PR easier:

  • .pre-commit-config.yaml
  • docs/conf.py
  • docs/package_description.py
  • projectq/backends/_exceptions.py
  • projectq/backends/_circuits/_plot.py
  • projectq/cengines/__init__.py
  • projectq/meta/_exceptions.py
  • projectq/setups/decompositions/phaseestimation.py

@Takishima Takishima requested a review from andreashehn June 30, 2021 11:29
@coveralls
Copy link

coveralls commented Jun 30, 2021

Pull Request Test Coverage Report for Build 1026758702

  • 325 of 325 (100.0%) changed or added relevant lines in 98 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 990359205: 0.0%
Covered Lines: 7000
Relevant Lines: 7000

💛 - Coveralls

@Takishima Takishima changed the title Modernize and fix projectq Modernize ProjectQ (isort, PEP 257 docstrings, drop Python 2 code) and fix phase estimation unit tests Jul 1, 2021
@Takishima Takishima changed the title Modernize ProjectQ (isort, PEP 257 docstrings, drop Python 2 code) and fix phase estimation unit tests Modernize ProjectQ (isort, PEP 257 docstrings, drop Python 2 code, more flake8 plugins) and fix phase estimation unit tests Jul 8, 2021
@Takishima Takishima self-assigned this Jul 12, 2021
Copy link
Collaborator

@andreashehn andreashehn left a comment

Choose a reason for hiding this comment

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

Looks good overall. I'd like you to address the comments in conf.py.

Everything else are just optional minor improvements for the docstings.
I find it a bit funny that a file "does" something, that's why I'd remove all the "Contains" and "Defines" and put in the docstring only a description of its contents (without a verb). For the "Registers", importing the file seems to actually do something, so I'd leave that.
But feel free to ignore and just resolve all those comments.

@Takishima
Copy link
Collaborator Author

Looks good overall. I'd like you to address the comments in conf.py.

Everything else are just optional minor improvements for the docstings.
I find it a bit funny that a file "does" something, that's why I'd remove all the "Contains" and "Defines" and put in the docstring only a description of its contents (without a verb). For the "Registers", importing the file seems to actually do something, so I'd leave that.
But feel free to ignore and just resolve all those comments.

Never thought about this in those terms, but I like your way of thinking. I'll keep that in mind for the future ;-)

@Takishima Takishima merged commit 1c07475 into ProjectQ-Framework:develop Jul 14, 2021
@Takishima Takishima deleted the modernize-and-fix-projectq branch July 14, 2021 17:12
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.

3 participants