tests: Clarify the property-based tests #85
Conversation
nbraud
changed the title
|
I just added a pair of commits that unify the variable names in the Hypothesis tests |
tests/test_vector2_angle.py
Outdated
| @@ -29,6 +29,7 @@ def test_angle(left, right, expected): | |||
| right=vectors(), | |||
| ) | |||
| def test_angle_range(left, right): | |||
| """Vector2.angle produces values in [-180; 180] and is antisymmetric.""" | |||
There was a problem hiding this comment.
Is antisymmetric different from asymmetric?
There was a problem hiding this comment.
Yes. Antisymmetric means (in that case) that left.angle(right) == - right.angle(left): it's symmetric, but with a twist (a sign change).
I will clarify that.
There was a problem hiding this comment.
BTW, I considered changing those vector names to x and y, since left and right aren't particularly meaningful there.
There was a problem hiding this comment.
Is dc0f688 enough clarification on antisymmetry?
Do you have an opinion on the variable name change I suggested?
There was a problem hiding this comment.
Names seem reasonable, or vector_a and vector_b.
Ehhh.... defining it doesn't actually feel better?
There was a problem hiding this comment.
What would you suggest then? Having a name for the property is useful, IMO (and that's a commonly-accepted name)
There was a problem hiding this comment.
I meant more that "antisymmetric" doesn't mean much to non-mathematicians.
Simply adding as an additional paragraph that flipping the order flips the sign is more helpful than trying to summarize it in a single word.
There was a problem hiding this comment.
Yes, and there is a clarification right afterwards:
def test_angle_range(left, right):
"""Vector2.angle produces values in [-180; 180] and is antisymmetric.
Antisymmetry means that left.angle(right) == - right.angle(left).
"""
|
I'm going to ask that @pathunstrom review this one because she's the one that originally filed the bug, and I understood the tests already. |
There was a problem hiding this comment.
I feel like as is, this hasn't actually approached what #75 was for. It did add clarification that allowed me to insert my own expansions of things, but for the most part the things that were the hardest for me to read before aren't any easier.
#75 is an accessibility issue. I have trouble parsing mathematic notation. In so many base formulas, I often have to do many layers of translation to be able to grasp what I'm looking at. My request for better naming is to do the expansion that me (and others like me) have to do by default to understand what's going on.
In rotation_linearity, for example, l is an arbitrary variable name, and scalar while still arbitrary, makes more sense that the meaning is only as a scalar and not some other intent.
|
@pathunstrom Thanks a lot for the feedback; I 100% agree that it is an accessibility issue, I just wasn't sure what to do to make the notation more accessible, beyond making it consistent, and your comment helped a lot 💜 |
nbraud
force-pushed
the
hypothesis/doc
branch
from
2897f00 to
b5b0134
Compare
|
PS: Rebased to solve the merge conflict with #88 |
nbraud
force-pushed
the
hypothesis/doc
branch
3 times, most recently
from
5eddee2 to
4edc9a6
Compare
|
I just rebased to fix the latest merge conflict. Is there any blocker left on this? I'm getting quite tired of needing to fix merge conflicts whenever any other PR touches the tests. |
nbraud
force-pushed
the
hypothesis/doc
branch
from
4edc9a6 to
720db9d
Compare
|
Rebased now that #97 is merged. |
|
@pathunstrom Is this progress or still as opaque to you? |
- x, y, ... are vectors - l, m, ... are scalars Closes ppb#75
nbraud
force-pushed
the
hypothesis/doc
branch
from
c71ce1a to
ff8b61f
Compare
|
Rebased again after the last merges. |
|
@astronouth7303 Can this go forward? |
This includes: - unused variables and imports; - ambiguous variable names which should have been fixed by ppb#85; - a confusing chain of equalities that likely didn't test what it was meant to.
This includes: - unused variables and imports; - ambiguous variable names which should have been fixed by ppb#85; - a confusing chain of equalities that likely didn't test what it was meant to.
This includes: - unused variables and imports; - ambiguous variable names which should have been fixed by ppb#85; - a confusing chain of equalities that likely didn't test what it was meant to.
This includes: - unused variables and imports; - ambiguous variable names which should have been fixed by ppb#85; - a confusing chain of equalities that likely didn't test what it was meant to.
This includes: - unused variables and imports; - ambiguous variable names which should have been fixed by ppb#85; - a confusing chain of equalities that likely didn't test what it was meant to.
111: CI: Run flake8 (plus various plugins) during tests r=astronouth7303 a=nbraud - Superset of #116 : - [x] Extract tests & QA tools execution to a `tests.sh` script. - [x] Reorder execution order to fail faster. - [x] Eliminate mutable default parameters (found by `flake8-bugbear`) - [x] Uniformize the ordeer of `import` lines. - [x] Uniformize the use of trailing commas - Automatically enforce the things that were fixed in #116 : - [x] Check basic lints with `flake8` - [x] Check for PEP 8 [naming conventions](https://www.python.org/dev/peps/pep-0008/#naming-conventions) This caught an issue that should have been fixed in #85 (a scalar named `l`); this likely happened because the test in question was added concurrently to #85. - [x] Enable `flake8-bugbear` and its “opinionated” checks This caught a number of instances where mutable data was used in default parameters. - [x] Check for trailing commas where useful - [x] Enforce a consistent import order [PEP 8]: https://www.python.org/dev/peps/pep-0008/ [`black`]: https://github.com/ambv/black Co-authored-by: Nicolas Braud-Santoni <nicolas@braud-santoni.eu>
111: CI: Run flake8 (plus various plugins) during tests r=astronouth7303 a=nbraud - Superset of #116 : - [x] Extract tests & QA tools execution to a `tests.sh` script. - [x] Reorder execution order to fail faster. - [x] Eliminate mutable default parameters (found by `flake8-bugbear`) - [x] Uniformize the ordeer of `import` lines. - [x] Uniformize the use of trailing commas - Automatically enforce the things that were fixed in #116 : - [x] Check basic lints with `flake8` - [x] Check for PEP 8 [naming conventions](https://www.python.org/dev/peps/pep-0008/#naming-conventions) This caught an issue that should have been fixed in #85 (a scalar named `l`); this likely happened because the test in question was added concurrently to #85. - [x] Enable `flake8-bugbear` and its “opinionated” checks This caught a number of instances where mutable data was used in default parameters. - [x] Check for trailing commas where useful - [x] Enforce a consistent import order [PEP 8]: https://www.python.org/dev/peps/pep-0008/ [`black`]: https://github.com/ambv/black Co-authored-by: Nicolas Braud-Santoni <nicolas@braud-santoni.eu>
@pathunstrom raised in #75 that the variable names used aren't consistent.
This
doesn't quite fix that (yet?), but at least itshould make it clearer what's being tested.I chose not to document the one-liner test, as a comment containing the same code as the assertion below it would be pretty useless.