-
Notifications
You must be signed in to change notification settings - Fork 1
Implement probability checks for native noise statements #184
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
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
3937433 to
32af32c
Compare
|
After discussion with @weinbe58: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create a shared implementation of the check put it outside the method? That would make testing a bit easier as well since you can check the implementation even if the statements do not run the verify.
32af32c to
f3cac3e
Compare
|
@weinbe58 I refactored things so now all the statements share the check. However, I still can't get the error to trigger in the CI, even after updating to kirin v0.17.2, even though I explicitly call @Roger-luo here's a kirin MWE on v0.17.2 that also never triggers the error raised in from kirin import ir, lowering
from kirin.decl import statement
from kirin.prelude import basic_no_opt
dialect = ir.Dialect("foo")
@statement(dialect=dialect)
class InvalidStmt(ir.Statement):
traits = frozenset({lowering.FromPythonCall()})
def check(self):
raise ValueError("Never triggers")
@ir.dialect_group(basic_no_opt.add(dialect))
def foo(self):
def run_pass(mt):
pass
return run_pass
@foo
def test():
InvalidStmt()
test.verify() |
weinbe58
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just address the comments but LGTM.
Closes #164
Note: still need to test it, blocked by the issue I have (similar to the one in #183 )