-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[Testing] Allow Capitalized name in CompareBeforeAfter #15568
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
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
Lunderberg
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.
Looks good, just one change to avoid accidental misuse. I like the change, and I'm always unsure how to name a @I.ir_module-annotated class. On the one hand, it looks like a class definition, but on the other hand, it produces something that acts more like a variable. Supporting both sounds reasonable to me.
| cls.before = cls._normalize_before(cls.before) | ||
| if hasattr(cls, "expected"): | ||
| cls.expected = cls._normalize_expected(cls.expected) | ||
| for name in ["before", "Before"]: |
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.
Can we add an assert that at most one of the two options are present?
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.
I'm not sure if should be that strong of an assert, as that would prevent defining an intermediate test that defines transform and expected, but doesn't define before. This could be useful for defining several tests that should all normalize to produce the same result. This would also break existing cases where the transform is defined, then used across several tests in a file.
What if instead, we were to assert that no duplicates names exist?
assert len([getattr(cls,name) for name in ['before','Before'] if hasattr(cls,name)]) <= 1
assert len([getattr(cls,name) for name in ['expected','Expected'] if hasattr(cls,name)]) <= 1
Lunderberg
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.
Thank you for making the changes, and LGTM!
This PR allows to use capitalized variable name
BeforeandExpectedinCompareBeforeAfter. Whenbefore / afterare classes or ir module, using capitalized variable names is more consistent with python naming convention.cc @Lunderberg