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

feat: add repeated structures #136

Merged
merged 14 commits into from
Nov 7, 2024
Merged

feat: add repeated structures #136

merged 14 commits into from
Nov 7, 2024

Conversation

mstechly
Copy link
Contributor

@mstechly mstechly commented Nov 5, 2024

Description

Adds support for repeated structures and a couple of other minor features.
For tests to pass it would require PsiQ/qref#124 to go in first an QREF to be released.

There's a couple of things left:

  • Documentation
  • Some minor test cases are missing
  • Moving _evaluate_and_define_functions in an appropriate place.

Please verify that you have completed the following steps

  • I have self-reviewed my code.
  • I have included test cases validating introduced feature/fix.
  • I have updated documentation.

@cla-bot cla-bot bot added the cla-signed label Nov 5, 2024
Copy link
Contributor

@dexter2206 dexter2206 left a comment

Choose a reason for hiding this comment

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

Overall looks good, and I only left minor suggestions, with a following caveat:

After reviewing this and the QREF's counterpart of this PR I'm more and more convinced that it would be better if we had a separate class representing "loop" or a "routine with repetitions" (or at least if we had it in Bartiq). This is because a lot of the logic we perform for validation and dispatching (with ifs) could be neatly hidden. For instance, we verify if routine with repetition has one child - but we could have an object that HAS to have one child (which would be validated by Pydantic). Anyway, we can continue with the current solution and later refactor it if we have some time to do so.

docs/concepts/compilation.md Outdated Show resolved Hide resolved
docs/concepts/compilation.md Outdated Show resolved Hide resolved
src/bartiq/_routine.py Outdated Show resolved Hide resolved
src/bartiq/repetitions.py Outdated Show resolved Hide resolved
src/bartiq/repetitions.py Outdated Show resolved Hide resolved
src/bartiq/repetitions.py Outdated Show resolved Hide resolved
src/bartiq/repetitions.py Outdated Show resolved Hide resolved
tests/compilation/test_repetitions.py Show resolved Hide resolved
tests/data/invalid_repetitions.yaml Outdated Show resolved Hide resolved
src/bartiq/symbolics/backend.py Show resolved Hide resolved
@mstechly mstechly merged commit 57312a1 into main Nov 7, 2024
7 checks passed
@mstechly mstechly deleted the repetitive-structures branch November 7, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants