Skip to content

Conversation

@johnzl-777
Copy link
Contributor

@johnzl-777 johnzl-777 commented Apr 8, 2025

┆Issue is synchronized with this Jira Task by Unito

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2025

PR Preview Action v1.6.0
Preview removed because the pull request was closed.
2025-04-10 18:16 UTC

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2025

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
6087 5256 86% 0% 🟢

New Files

File Coverage Status
src/bloqade/squin/analysis/nsites/_init_.py 100% 🟢
src/bloqade/squin/analysis/nsites/analysis.py 84% 🟢
src/bloqade/squin/analysis/nsites/impls.py 93% 🟢
src/bloqade/squin/analysis/nsites/lattice.py 83% 🟢
TOTAL 90% 🟢

Modified Files

File Coverage Status
src/bloqade/squin/op/stmts.py 100% 🟢
src/bloqade/squin/op/traits.py 59% 🟢
TOTAL 79% 🟢

updated for commit: 2da07d3 by action🐍

@codecov
Copy link

codecov bot commented Apr 8, 2025

Codecov Report

Attention: Patch coverage is 86.72566% with 15 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/bloqade/squin/analysis/nsites/lattice.py 82.75% 5 Missing ⚠️
src/bloqade/squin/analysis/nsites/analysis.py 84.00% 4 Missing ⚠️
src/bloqade/squin/analysis/nsites/impls.py 93.02% 3 Missing ⚠️
src/bloqade/squin/op/traits.py 57.14% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@johnzl-777 johnzl-777 marked this pull request as ready for review April 9, 2025 18:18
@johnzl-777
Copy link
Contributor Author

Hi @Roger-luo I believe this is a reasonable implementation of the squin operator shape analysis. I understand you also mentioned a "propagation of unitarity" in #11 but from what I can tell this would require something similar to a const prop analysis + wrap const rewrite? As in I'd have something like a UnitaryAnalysis pass and then a rewrite to set the traits.

It just doesn't seem very idiomatic to me to try and kill two birds with one stone in analysis here.

If the behavior you see in squin_op_playground.py is correct I can go ahead and make unit tests before this gets merged in 👍

Copy link
Collaborator

@Roger-luo Roger-luo left a comment

Choose a reason for hiding this comment

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

neat PR, just some minor revision needed, great work!

@Roger-luo Roger-luo added priority: high high priority, blocking milestones, time sensitive enhancement New feature or request squin squin related issues labels Apr 9, 2025
@Roger-luo Roger-luo added this to the v0.1.0 milestone Apr 9, 2025
@johnzl-777 johnzl-777 changed the title Squin Operator Shape Analysis Squin Operator Sites Analysis Apr 10, 2025
@johnzl-777 johnzl-777 requested a review from Roger-luo April 10, 2025 15:53
@johnzl-777
Copy link
Contributor Author

johnzl-777 commented Apr 10, 2025

@Roger-luo I did my best to implement the name changes you requested but also did my best to pick some names that were still clear enough that one can disambiguate between them.

Credit to @weinbe58 for helping me think through some of the names in discussion!

I also added unit tests to try to cover all the proper codepaths (:

Copy link
Collaborator

@Roger-luo Roger-luo left a comment

Choose a reason for hiding this comment

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

nice! I think the code is great to go! Just remmeber to cleanup your playground

@@ -0,0 +1,59 @@
from kirin import ir, types
Copy link
Collaborator

Choose a reason for hiding this comment

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

you wanna delete the playground?

Copy link
Contributor Author

@johnzl-777 johnzl-777 Apr 10, 2025

Choose a reason for hiding this comment

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

Yes! Thank you for catching that. I should probably do a local ignore for any _playground.py files moving forward

Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually use a main.py as my playground it's in the .gitignore already IIRC

@johnzl-777 johnzl-777 merged commit 3b788ca into main Apr 10, 2025
12 checks passed
@johnzl-777 johnzl-777 deleted the john/squin-operator-shape-analysis branch April 10, 2025 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request priority: high high priority, blocking milestones, time sensitive squin squin related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants