CadQuery / cadquery

A python parametric CAD scripting framework based on OCCT
https://cadquery.readthedocs.io
Other
3.22k stars 293 forks source link

Investigate multimethods #379

Open adam-urbanczyk opened 4 years ago

adam-urbanczyk commented 4 years ago

We might want to consider using this https://pypi.org/project/multimethod/ to make the CQ code more readable.

marcus7070 commented 4 years ago

Are you suggesting this because having positional arguments with multiple types would be closer to the OCP/OCCT function signatures?

Can you give any examples functions in the code that could be improved by multimethods?

marcus7070 commented 4 years ago

Nevermind, your commit here makes things more clear. https://github.com/CadQuery/cadquery/pull/378/commits/ca459ae9afd1d1b05556622e41324d9d19fd0a3c Init methods in particular look like good targets, such as init-ing a transform off another matrix or a series of floats. I can also see the potential when multiplying a vector by a float or a matrix.

marcus7070 commented 4 years ago

I was curious about docstrings when using multimethods. Looks like you write one docstring per method and the module combines all those docstrings with the associated signature into a single string that would be displayed by your IDE/language server.

https://github.com/coady/multimethod/blob/b15cff777840c9e5231c8b27e168ea3dd14ebb57/multimethod/__init__.py#L197

IMHO that would be a huge improvement for the readability of the CQ codebase. I just hope it plays nicely with Sphinx!

marcus7070 commented 3 years ago

exporters.export might also be a good target for this, probably with a slight change to the arguments to give multimethod something to dispatch on.

https://github.com/CadQuery/cadquery/blob/dc9b0e70d15e04c6fa9eafc4f181646f4a13c774/cadquery/occ_impl/exporters/__init__.py

marcus7070 commented 3 years ago

I've had a play around with the multimethod and multipledispatch packages and I can't get type inference working with them. Here is a MWE:

fluent_test.py:

from multimethod import multimethod

from typing import TypeVar

T = TypeVar("T", bound="Test")
T2 = TypeVar("T2", bound="Vanilla")

class Test:

    def __init__(self) -> None:
        pass

    @multimethod  # type: ignore[misc]
    def f(self: T, a: int) -> T:
        self.i = a
        return self

    @f.register
    def _str(self: T, a: str) -> T:
        self.s = a
        return self

class Vanilla:

    def __init__(self) -> None:
        pass

    def f(self: T2) -> T2:
        return self

t0 = Test()
reveal_type(t0)
t1 = t0.f(1)
reveal_type(t1)

t2 = Vanilla()
reveal_type(t2)
t3 = t2.f()
reveal_type(t3)
> mypy fluent_test.py
fluent_test.py:35: note: Revealed type is 'fluent_test.Test'
fluent_test.py:37: note: Revealed type is 'Any'
fluent_test.py:40: note: Revealed type is 'fluent_test.Vanilla'
fluent_test.py:42: note: Revealed type is 'fluent_test.Vanilla*'

That Any after a multimethod is unacceptable. I also get the same behaviour if you try to multimethod an __init__ (ie. if Vector.__init__ is changed to a set of multimethods, x = Vector() has inferred type Any).

I am very far from a mypy expert, so please correct me if you can see any mistakes or methods to get type inference working.

adam-urbanczyk commented 3 years ago

Does this https://github.com/coady/multimethod/pull/37 solve the issue?

marcus7070 commented 3 years ago

I missed that, thanks. I was working from v1.5. Unfortunately the current main (https://github.com/coady/multimethod/commit/aa99df03e0d5254f2bfee440477aeed6621e50bb) still doesn't get there. Here is a cleaner MWE:

chain_test.py:

from typing import TypeVar
from multimethod import multimethod

T0 = TypeVar('T0', bound='Shape')
T1 = TypeVar('T1', bound='MMShape')

class Shape:
    def set_scale(self: T0, scale: float) -> T0:
        self.scale = scale
        return self

class MMShape:
    @multimethod
    def set_scale(self: T1, scale: float) -> T1:
        self.scale = scale
        return self

s0 = Shape().set_scale(1.0)
reveal_type(s0)

s1 = MMShape().set_scale(1.0)
reveal_type(s1)
> mypy chain_test.py
chain_test.py:22: note: Revealed type is 'chain_test.Shape'
chain_test.py:25: note: Revealed type is 'Any'
adam-urbanczyk commented 3 years ago

Hm, too bad. I'll take a look how difficult it is to fix this.

adam-urbanczyk commented 3 years ago

Did not really look, but at least opened an issue...