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

🎨 Improved usability of the on-the-fly SiDB gate library #634

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

Conversation

Drewniok
Copy link
Collaborator

Description

This PR improves the usability of the on-the-fly SiDB gate library. Additionally, several bugs have been fixed.

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have added a changelog entry.
  • I have created/adjusted the Python bindings for any new or updated functionality.
  • 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.

@Drewniok Drewniok added the enhancement New feature or request label Jan 15, 2025
@Drewniok Drewniok self-assigned this Jan 15, 2025
Drewniok and others added 5 commits January 15, 2025 17:12
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@Drewniok Drewniok requested a review from marcelwa January 31, 2025 17:19
@Drewniok
Copy link
Collaborator Author

Drewniok commented Feb 1, 2025

I think the coverage is pretty good now. What do you think?

Copy link
Collaborator

@marcelwa marcelwa left a comment

Choose a reason for hiding this comment

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

Just a few comments and questions here and there. Nothing critical, as far as I can tell 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's great to have such test resources available. Do you think we can organize them in this folder more meaningfully for easer extension in the future? AND_layout.sqd could be anything. Path and filename don't give away that it is a SiQAD gate (if it is) or what makes it special or what test case it is used for. Maybe I can propose something along the lines of test/resources/simulation/sidb/siqad/AND.sqd. This might not be perfect, so please incorporate changes if required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right! It is crucial to get more structure here so as not to lose the overview. Thank you!

@Drewniok Drewniok requested a review from marcelwa February 6, 2025 09:19
Signed-off-by: GitHub Actions <[email protected]>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@Drewniok
Copy link
Collaborator Author

Drewniok commented Feb 7, 2025

@marcelwa I would love to hear your feedback when your time allows it!

@marcelwa
Copy link
Collaborator

marcelwa commented Feb 7, 2025

@marcelwa I would love to hear your feedback when your time allows it!

I'm on it.

Copy link
Collaborator

@marcelwa marcelwa left a comment

Choose a reason for hiding this comment

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

As before, just a few minor comments here and there. Should be quick to address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants