Refactored math tests to iterate over all math ops #21511
Conversation
Added similar logic to __init__.py Formatting Added compatibility for python2
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
ailzhang
left a comment
ailzhang
left a comment
There was a problem hiding this comment.
Refactor in __init__.py looks great, my only concern is about the if-else block in test.
test/test_jit.py
Outdated
|
|
||
| unimplemented = ["fsum", "isclose", "trunc", "hypot", "log2"] | ||
| ops = [x for x in dir(math) if callable(getattr(math, x))] | ||
| for op in ops: |
There was a problem hiding this comment.
well, really, we needed to have a bunch of special cases for calling the different tests anyways. So for each special case for calling the tests, we have an additional if branch, so I don't think it's too bad.
There was a problem hiding this comment.
I think it's going to be messy no matter what. I guess my general preference is to avoid relying on python implementation details when possible, which we're doing here by calling dir. That has been safe so far but who knows what future python versions will do. (Although we do that in other places too @driazati)
In this case i guess slight preference towards not relying on dir, but it's test code and kind of arbitrary and messy so i think it's up to the author here.
There was a problem hiding this comment.
I think the upside of not needing to manually compare each function in the math documentation
with our implementations to see what's implemented is worth the downside of relying on python implementation details. For example, the original issue missed the functions log2 and hypot, presumably because they added them in largely manually.
I also don't think dir is really a "python implementation detail". We use it all over the place, and not just in tests.
There was a problem hiding this comment.
dir is picking up attributes that are not public facing methods of the module such as __loader__. That feels like implementation details to me. Where else do we use dir? I know we do in some places where there isn't an alternative
There was a problem hiding this comment.
dir doesn't get implementation details, everything it gets is well-defined. Ignoring __whatever__ will leave us with just the public math API, which is what we want
There was a problem hiding this comment.
Because dir() is supplied primarily as a convenience for use at an interactive prompt, it tries to supply an interesting set of names more than it tries to supply a rigorously or consistently defined set of names, and its detailed behavior may change across releases. For example, metaclass attributes are not in the result list when the argument is a class.
It's not a stable API
test/test_jit.py
Outdated
|
|
||
| unimplemented = ["fsum", "isclose", "trunc", "hypot", "log2"] | ||
| ops = [x for x in dir(math) if callable(getattr(math, x))] | ||
| for op in ops: |
There was a problem hiding this comment.
I think it's going to be messy no matter what. I guess my general preference is to avoid relying on python implementation details when possible, which we're doing here by calling dir. That has been safe so far but who knows what future python versions will do. (Although we do that in other places too @driazati)
In this case i guess slight preference towards not relying on dir, but it's test code and kind of arbitrary and messy so i think it's up to the author here.
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Refactored math tests to iterate over all math ops Added similar logic to __init__.py Formatting Added compatibility for python2 gh-metadata: pytorch pytorch 21511 gh/chillee/35/head
Summary: Pull Request resolved: pytorch#21511 Test Plan: Imported from OSS Differential Revision: D16088193 Pulled By: Chillee fbshipit-source-id: 81b6e536b4505178c829a9d925c30cd185b7a706
Stack from ghstack:
Added similar logic to init.py
Formatting
Added compatibility for python2
Differential Revision: D16088193