-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Turn on exact_dtype by default on test_optim.py #34825
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
[ghstack-poisoned]
test/test_optim.py
Outdated
class TestOptim(TestCase): | ||
def setUp(self): | ||
super(TestOptim, self).setUp() | ||
self.exact_dtype = True |
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.
Don't do this; exact_dtype
is intended to be a class global property. Just set exact_dtype = True
directly in the class, like in the other PR.
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 my understanding is correct. Doing self.exact_dtype = True
in a class member function should be equivalent to directly set exact_dtype = True
in the class? Also when we modify the inherited attribute exact_dtype, I guess it won't affect the attribute value of the parent's other subclasses, since they instantiate a different class object.
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.
No it is not equivalent (although in this particular case I can believe that they are functionally equivalent)
ezyang-mbp:Dev ezyang$ python arf.py
True
Traceback (most recent call last):
File "arf.py", line 9, in <module>
print(A2.b)
AttributeError: class A2 has no attribute 'b'
ezyang-mbp:Dev ezyang$ cat arf.py
class A:
b = True
class A2:
def __init__(self):
self.b = True
print(A.b)
print(A2.b)
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 promising!
💊 CircleCI build failures summary and remediationsAs of commit 85fee57 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no CircleCI failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker. This comment has been revised 17 times. |
[ghstack-poisoned]
[ghstack-poisoned]
@great-way merged this pull request in 5857a12. |
Stack from ghstack:
Differential Revision: D20498111