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

Lightsout updates #227

Open
wants to merge 92 commits into
base: main
Choose a base branch
from
Open

Lightsout updates #227

wants to merge 92 commits into from

Conversation

lucasberent
Copy link
Collaborator

@lucasberent lucasberent commented May 14, 2024

Description

Includes recent updates for the liightsout decoder and stim integration.

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

@burgholzer burgholzer added enhancement Enhancement to existing feature python Pull requests that update Python code minor Minor version update labels May 14, 2024
Signed-off-by: burgholzer <burgholzer@me.com>
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this 🙏🏼
I quickly went through everything and adjusted a couple of things myself.
Most importantly, I moved all of the evaluation code outside of the main package and into the scripts directory. Files like that typically do not belong in the package itself.

Besides the inline comments below, this PR is completely missing tests.
None of the stim integration is tested, which makes it quite hard to figure out whether it is actually working as intended.
If the stim interface should be part of the package (which probably makes sense), it should be fully tested to ensure future compatibility.

I'd also advise to re-run all the plotting scripts and other evaluation scripts at least once after all changes have been incorporated in order to check whether they still work.

Hopefully, that does not consume too much time 😌

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/mqt/qecc/cc_decoder/decoder.py Outdated Show resolved Hide resolved
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Thanks for your iteration on the code! Almost done.

Please try to get the CI to an all-green state. The sinter part of this PR is not yet covered by any tests. I am pretty sure you will notice that something is missing once you start writing the tests ;-)

Also, please do not needlessly add linter ignores. Especially not when the linter already tells you precisely what to do/fix and it's just a hand full of warnings to address.
Did that now in 330830c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to existing feature minor Minor version update python Pull requests that update Python code
Projects
Status: In Progress
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants