Fix return types #71

AstraLuma · 2018-10-20T19:32:49Z

Actual algorithm for mixed-type operations
Fix more annotations
Tests to verify return types

There is an argument to be made for Vector/Vector operations to return the left type.

nbraud · 2018-10-20T21:33:46Z

@astronouth7303 I don't understand what's going on.

Is it making vector/vector operations that involve subclasses of Vector2 return the subclass? If so, what's the motivation?

AstraLuma · 2018-10-20T21:55:55Z

Consistency of typing without every subclass needing to wrap each operation to return the right type.

Concretely, so ppb.vector.Vector appears and avoiding Vector2 showing up.

nbraud · 2018-10-20T21:57:02Z

Oh, I didn't realise ppb.Vector was different from Vector2.

👍 then, as it causes the bug I mentioned in mutant-game

nbraud · 2018-10-20T21:57:53Z

The type annotations are probably not as precise as they could be, though.
(Because they don't reflect that we return something more precise than Vector2)

AstraLuma · 2018-10-20T22:00:32Z

Subclasses of the thing are implied in all annotations, afaik.

So, they all return a Vector2. It might be a specialization of Vector2, but it'll be a Vector2.

nbraud · 2018-10-20T23:06:30Z

Yes, but what I mean is that code that does a vector operation, then calls a subclass method/property on the result, would work with your change, but fail to typecheck because the typechecker thinks we return a Vector2

AstraLuma · 2018-10-20T23:33:23Z

https://mypy.readthedocs.io/en/latest/kinds_of_types.html#class-types

Any instance of a subclass is also compatible with all superclasses

So a type annotation of Vector2 (or its late-bound variant 'Vector2') implicitly includes all subclasses of Vector2.

nbraud · 2018-10-20T23:39:54Z

@astronouth7303 OK, to clarify:

class V(Vector2):
    def method(self):
        pass

x = V(1, 0) + (0, 1)  # x's inferred type will be Vector2, not V
x.method()  # Should fail to typecheck

The type annotation is valid (as you said, a (covariant) annotation of Vector2 includes its subtypes), but it's not precise enough to be useful when people subclass Vector2.

AstraLuma · 2018-10-21T00:09:28Z

Oh, then yes. I think this is one of the limitations of the python type checker, since (afaik), you can't have a type variable that goes between different bits of a method's annotation.

Like you can't say "x.foo() will return whatever type x is".

pathunstrom · 2018-10-21T00:36:13Z

That sounds like a thing that should be discussed on mypy.

tests/test_typing.py

pathunstrom · 2018-10-21T00:43:34Z

tests/test_typing.py

+    # Normalize for reflect
+    a = op(V1(1, 2), V2(3, 4).normalize())
+    b = op(V2(1, 2), V1(3, 4).normalize())
+    assert isinstance(a, V2)


Do you mean specifically that we're returning the subclass in all cases?

Yes, because V2 is a subclass of V1, it should return the more specialized V2.

ppb_vector/vector2.py

pathunstrom · 2018-10-21T00:48:51Z

ppb_vector/vector2.py

@@ -34,6 +35,37 @@ def _mkvector(value, *, castto=_fakevector):
        raise ValueError(f"Cannot use {value} as a vector-like")


+@functools.lru_cache()
+def _find_lowest_type(left: type, right: type) -> type:


Should probably use typing.Type here.

These are documented as equivalent:

Type[Any] is equivalent to Type which in turn is equivalent to type, which is the root of Python’s metaclass hierarchy.

But I'll do it because metaprogramming like this can be weird.

nbraud · 2018-10-21T03:04:38Z

@astronouth7303 It looks like it's doable: https://gist.github.com/nbraud/a592985bb700ac74894efd8243cb08ce

AstraLuma · 2018-10-22T21:30:46Z

Everything mentioned so far should be taken care of.

AstraLuma · 2018-10-22T22:56:56Z

With annotations getting this complex, I figured we should actually run it through mypy.

It was informative.

AstraLuma · 2018-10-23T01:47:08Z

python/mypy#5821 came up, and has some insights into typing.

AstraLuma · 2018-10-23T02:08:26Z

Sorry, this PR is getting kinda large. I feel like I've touched most of the code at this point.

nbraud · 2018-10-27T19:02:09Z

@astronouth7303 Yeah, there is likely no way to improve the annotations without touching all the methods. Thanks a lot for doing it.

Small question about Realish (which I pronounce “relish” in my head, it's funny): I had also noticed that mypy complains about floats and ints not being Reals, but CPython seems to disagree:

> from numbers import Real

> isinstance(1, Real)
True

> isinstance(1.0, Real)
True

Is mypy in the wrong about ints and floats not being Reals?

nbraud · 2018-10-27T19:03:03Z

Re: the PR becoming too large, I guess we can split off the actual behaviour change (in selecting the return type) and the improvement in type annotations. What do you think?

AstraLuma · 2018-10-29T18:56:20Z

I suppose I can. I'm not sure it helps much?

AstraLuma · 2018-10-29T18:58:58Z

Small question about Realish (which I pronounce “relish” in my head, it's funny): I had also noticed that mypy complains about floats and ints not being Reals, but CPython seems to disagree:

This is because mypy doesn't pickup on runtime registered ABC subclasses.

So, mypy is correct in that numbers.Real is not in the MRO of int and float. But, if you ask if int is a subclass of Real at runtime, the answer will be yes (because it's explicitly registered as such).

(And no, this is pretty impure typing. It comes from a pragmatic side because numbers really should be a Python module, but any Python code that is core to CPython--such as a bunch of the import machinery--has to be handled Very Special. And for something as core as int and float, I'm just not sure its possible to import a Python module before int is defined.)

nbraud · 2018-10-29T20:00:22Z

@astronouth7303 Sounds like a bug that should be reported against mypy, then?
In any case, we should definitely keep the Realish union for now, but if the mypy devs fix it, we can in the future remove it.

AstraLuma · 2018-10-29T20:09:37Z

That's Real Complicated, and there's been Very Large Discussions about mypy and ABCs and The Number Tower (PEP 3141). See python/mypy#2636 and python/mypy#3186 for details on those discussions.

.travis.yml

ppb_vector/vector2.py

nbraud · 2018-10-29T20:14:33Z

ppb_vector/vector2.py

+        elif isinstance(value, Vector2):
+            return cls(value.x, value.y)
+        # FIXME: Allow all types of sequences
+        elif isinstance(value, (list, tuple)) and len(value) == 2:


isinstance(value, typing.Sequence) ?

You'd want collections.abc.Sequence

What's the difference, concretely? (Also, sure, go for it)

typing.Sequence is basically a typing-related data structure, only used to construct annotation data.

collections.abc.Sequence is an actual abstract class with default implementations of a pile of methods.

nbraud · 2018-10-29T20:17:03Z

ppb_vector/vector2.py

+        elif isinstance(value, (list, tuple)) and len(value) == 2:
+            return cls(value[0].__float__(), value[1].__float__())
+        # FIXME: Allow all types of mappings
+        elif isinstance(value, dict) and 'x' in value and 'y' in value and len(value) == 2:


isinstance(value, typing.Mapping)?

Also, do we want to enforce len(value) == 2?
It might be useful to be able to build a vector from a dict like {'x': 0, 'y': 1, 'color': 'brown'}.

For now, I think we want to keep exact forms to prevent mistakes.

Especially since afaik no code actually uses the dict form.

You added Mapping to the imports in a6161b9 but you forgot to do anything with it.

I did, this comment is outdated.

nbraud · 2018-10-29T20:18:02Z

ppb_vector/vector2.py

+
+    @property
+    def length(self) -> float:
+        if not hasattr(self, '_length'):


Is the reflection dance with hasattr to memoize the vector length really faster than recomputing it every time?

OK, I did a quick μbench with timeit, and it looks like the speedup/slowdown of saving the result in _length is below the measurement noise, so I would recommend removing that code and just doing return hypot(self.x, self.y).

$ ./dunder_bench.py 1 successive length computations Memoized: 0.41153700300492346 Computed: 0.41805144600220956 Speedup: 1.5829543758445919% 5 successive length computations Memoized: 1.2452478090126533 Computed: 1.2318236450082622 Speedup: -1.0780315297269971% 10 successive length computations Memoized: 2.252152765984647 Computed: 2.2965358549845405 Speedup: 1.970696200996347% 25 successive length computations Memoized: 5.274616469978355 Computed: 5.400348082999699 Speedup: 2.3837110003537276% 50 successive length computations Memoized: 10.367006862012204 Computed: 10.45787951600505 Speedup: 0.8765563214376826% 100 successive length computations Memoized: 21.056249847984873 Computed: 21.302848722989438 Speedup: 1.1711433744606925% ./dunder_bench.py 81.75s user 0.00s system 99% cpu 1:21.77 total

Does computing it have any significant overhead overhead? I did this to get rid of _fakevector while keeping the performance benefit of not computing the length at construction time.

@astronouth7303 That's what the microbenchmark I wrote was showing: even if you access the computed length 100 times in a row, loading it from self._length isn't faster than recomputing it.

I simplified the implementation of length.

nbraud · 2018-10-29T20:20:49Z

ppb_vector/vector2.py

        except ValueError:
            return NotImplemented
-        rtype = type(other) if isinstance(other, Vector2) else type(self)


Is it a good idea to move the computation of rtype before the try/except?
It can be that _find_lowest_vector throws an exception in cases where we would previously have returned NotImplemented

_find_lowest_vector() doesn't throw exceptions, and they are internal functions meant for DRYing.

ppb_vector/vector2.py

nbraud · 2018-10-29T20:26:46Z

tests/test_vector2_angle.py

@@ -10,7 +10,7 @@
    (Vector2(1, 1), Vector2(-1, 0), 135),
    (Vector2(0, 1), Vector2(0, -1), 180),
    (Vector2(-1, -1), Vector2(1, 0), 135),
-    (Vector2(-1, -1), Vector2(-1, 0), 45),
+    (Vector2(-1, -1), Vector2(-1, 0), -45),


Why are there unrelated changed in the angle tests?

Because passing tests.

Except that the tests are passing in master, so there is something wrong going on.

You're going to make my life difficult, I know it.

OK.

This is actually a bug mypy found.

The two tests in this file had the same name, so only the latter one was being run.

This exposed problems in the earlier test because it wasn't running before.

This can be moved to its own PR if y'all would really rather.

tests/test_vector2_reflect.py

nbraud · 2018-10-29T21:09:56Z

@astronouth7303 Thanks for the link to the mypy discussions, re: the numbers tower.

pathunstrom · 2018-11-01T17:24:55Z

ppb_vector/vector2.py

+        """
+        return type(self)(self.x * other, self.y * other)
+
+    @typing.overload


AstraLuma added 6 commits October 20, 2018 15:09

Use type(self) as a constructor

9814f35

Improve the algorithm for selecting a type when two subclasses are given

11aec9d

Handle the common case when the two vectors are the same type

fe2462b

Fix some annotations

678be08

Fix some more annotations

f9d6eb9

Add tests for typing

06f64c8

AstraLuma requested a review from pathunstrom October 20, 2018 19:33

pathunstrom reviewed Oct 21, 2018

View reviewed changes

AstraLuma added 2 commits October 20, 2018 21:02

Use typing.Type instead of type

2ae4f56

test_typing: Use more explicit constant names

0cee3a2

Add subclass-aware annotations

f07e923

AstraLuma added 7 commits October 22, 2018 17:44

Run mypy in travis

118abfc

Bunch of mypy refactoring, add scale_by and scale_to.

86858d3

Fix convert, add tests

40b8ee5

Tell mypy to ignore pytest

dc21322

mypy: Shut up about __index__

8a2d181

Fix angle tests

e87798f

More annotation fixups

a055743

AstraLuma added 3 commits October 22, 2018 18:40

Merge remote-tracking branch 'origin/master' into fix-return-types

f17b0ee

More typing updates

66ec631

Better handling of types and errors in __init__

b0196c5

Be specific in attributes, because It Just Helps

f4e0b77

AstraLuma added 4 commits October 22, 2018 21:53

Use overload

a218218

Move to lazy .length, nullifying the need for a bunch of code

812cf6b

Fold _mkvector() into Vector2.convert()

e3e2a46

Make sure to compute the return type before we cast other

2052d8f

nbraud requested changes Oct 29, 2018

View reviewed changes

AstraLuma added 3 commits October 30, 2018 18:06

Use generic sequence/mapping

a6161b9

Don't duplicate code into __ne__()

ef6ae72

Undo unnecessary conversions to .dot()

f5e23d7

pathunstrom reviewed Nov 1, 2018

View reviewed changes

ppb_vector/vector2.py

"""

return type(self)(self.x * other, self.y * other)

@typing.overload

Copy link

Collaborator

pathunstrom Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

AstraLuma added 2 commits November 1, 2018 21:04

Don't cache the length, doesn't actually provide any benefits

5224a2e

Merge branch 'master' into fix-return-types

9333fda

AstraLuma force-pushed the fix-return-types branch from 49962d8 to 9333fda Compare November 2, 2018 02:55

AstraLuma merged commit 96883e0 into master Nov 16, 2018

AstraLuma deleted the fix-return-types branch November 16, 2018 07:35

nbraud mentioned this pull request Dec 6, 2018

Vector2: Remove superfluous _length attribute #78

Merged

Fix return types #71

Fix return types #71

Conversation

AstraLuma commented Oct 20, 2018

nbraud commented Oct 20, 2018

AstraLuma commented Oct 20, 2018

nbraud commented Oct 20, 2018

nbraud commented Oct 20, 2018 • edited Loading

AstraLuma commented Oct 20, 2018

nbraud commented Oct 20, 2018

AstraLuma commented Oct 20, 2018 • edited Loading

nbraud commented Oct 20, 2018 • edited Loading

AstraLuma commented Oct 21, 2018

pathunstrom commented Oct 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbraud commented Oct 21, 2018

AstraLuma commented Oct 22, 2018

AstraLuma commented Oct 22, 2018

AstraLuma commented Oct 23, 2018

AstraLuma commented Oct 23, 2018

nbraud commented Oct 27, 2018

nbraud commented Oct 27, 2018

AstraLuma commented Oct 29, 2018

AstraLuma commented Oct 29, 2018 • edited Loading

nbraud commented Oct 29, 2018

AstraLuma commented Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AstraLuma Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbraud commented Oct 29, 2018

Choose a reason for hiding this comment

nbraud commented Oct 20, 2018 •

edited

Loading

AstraLuma commented Oct 20, 2018 •

edited

Loading

nbraud commented Oct 20, 2018 •

edited

Loading

AstraLuma commented Oct 29, 2018 •

edited

Loading

AstraLuma commented Oct 29, 2018 •

edited

Loading

AstraLuma Oct 30, 2018 •

edited

Loading