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

[Testing] Allow Capitalized name in CompareBeforeAfter #15568

Merged
merged 3 commits into from
Aug 17, 2023

Conversation

vinx13
Copy link
Member

@vinx13 vinx13 commented Aug 15, 2023

This PR allows to use capitalized variable name Before and Expected in CompareBeforeAfter. When before / after are classes or ir module, using capitalized variable names is more consistent with python naming convention.

cc @Lunderberg

@tvm-bot
Copy link
Collaborator

tvm-bot commented Aug 15, 2023

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.

  • No users to tag found in teams: testing See #10317 for details

Generated by tvm-bot

@github-actions github-actions bot requested a review from Lunderberg August 15, 2023 22:49
Copy link
Contributor

@Lunderberg Lunderberg left a 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"]:
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor

@Lunderberg Lunderberg left a 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!

@Lunderberg Lunderberg merged commit 98320ab into apache:main Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants