Skip to content

Stub for fractions #94

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

Closed
wants to merge 2 commits into from
Closed

Stub for fractions #94

wants to merge 2 commits into from

Conversation

youtux
Copy link
Contributor

@youtux youtux commented Feb 26, 2016

I was trying to create the stub for the Python 3.4 statistics module, but it depends on the fractions module.
So here is my attempt for the fractions module, any suggestion accepted.

@youtux
Copy link
Contributor Author

youtux commented Feb 26, 2016

Done, thank you @matthiaskramm

@youtux
Copy link
Contributor Author

youtux commented Feb 26, 2016

Hmm, I don't really know how to deal with the first error:

running mypy  # with 207 files
stdlib/2and3/fractions.pyi: note: In class "Fraction":
stdlib/2and3/fractions.pyi:26: error: Signature of "__round__" incompatible with supertype "Real"

Perhaps it is necessary to change the stub of numbers.Real?

Regarding the second error:

running mypy --py2 # with 243 files
stdlib/2and3/fractions.pyi:3: error: No library stub file for standard library module 'decimal'
stdlib/2and3/fractions.pyi:3: note: (Stub files are from https://github.com/python/typeshed)

It is pretty obvious that the stub for the decimal module needs to be ported to python 2.
I would like to work on that. Should I start by moving decimal.pyi to the 2and3 directory?

@matthiaskramm
Copy link
Contributor

Yes, that's a good place for it. The Python 2 and Python 3 versions don't seem to differ much. Feel free to just mark the Python 3 specific sections with comments. Once mypy has support for version conditionals we can do the proper

  if sys.version_info[0] == 3:
   ...

@youtux
Copy link
Contributor Author

youtux commented Feb 26, 2016

Feel free to just mark the Python 3 specific sections with comments. Once mypy has support for version conditionals we can do the proper

sys.version_info[0] == 3:
    ...

I don't quite get your comment. Do you mean that I should comment out the following code?

    if sys.version_info[0] >= 3:
        def floor(self) -> int: ...
        def ceil(self) -> int: ...

        @overload
        def __round__(self) -> int: ...

        @overload
        def __round__(self, ndigits: int) -> Fraction: ...

If that is the case, the stub would not be checked at all, am I wrong?

@gvanrossum
Copy link
Member

Re: the signature of __round__: instead of using @overload, just use a default argument, i.e.

def __round__(self, ndigits: int = None): ...

Regarding the sys.version-info check, IIUC the proposal is to just leave out the if line and replace it with a comment saying soemthing like "TODO: this is for Python 3 only; fix when mypy supports sys.version_info checks"

@gvanrossum
Copy link
Member

I've merged your decimal backport; can you rebase and clean up?

@youtux
Copy link
Contributor Author

youtux commented Feb 26, 2016

Yeah, just waiting for that.

@youtux
Copy link
Contributor Author

youtux commented Feb 26, 2016

Please, don't merge. There are missing properties of Fraction.

@gvanrossum
Copy link
Member

OK, I'm waiting.

@youtux
Copy link
Contributor Author

youtux commented Feb 26, 2016

Should the stub also declare types for methods like __add__, __sub__, etc.? I suppose yes.

@gvanrossum
Copy link
Member

Regarding __add__ etc., these signatures should really be inherited from Number. You can'y say e.g.

def __add__(self, other: Fraction) -> Fraction: ...

Because it actually supports other numbers too. So you'll have to write something like

def __add__(self, other: Number) -> Number: ...

This is unfortunate because it means that type-checking expressions involving Fractions is not very precise. We could add overloads but it would get pretty verbose... E.g.

@overload
def __add__(self, other: Union[Fraction, int]) -> Fraction: ...
@overload
def __add__(self, other: Number) -> Number: ...

but I'm not even sure if mypy currently support that -- please try it out with some real code. Thanks for bringing it up!

@youtux
Copy link
Contributor Author

youtux commented Feb 26, 2016

What about something like:

NumberType = TypeVar('T', Fraction, numbers.Real, numbers.Complex)

class Fraction(numbers.Rational):
    ...
    def __add__(self, other: NumberType) -> NumberType: ...

The rationale is that Fraction.__add__ seems to cast self to the upper category (float, complex) until they can be added.

@gvanrossum
Copy link
Member

gvanrossum commented Feb 26, 2016 via email

@youtux
Copy link
Contributor Author

youtux commented Feb 27, 2016

I don't quite get why mypy complains.

@gvanrossum
Copy link
Member

I think you're running into something that's unsupported currently by mypy.

@JukkaL can you shed your light on this issue? The key problem seems to be that the base class defines e.g.

def __add__(self, other: Number) -> Number: ...

and in a subclass we'd like to override this so that mypy understands that adding two Fractions returns a Fraction, but that adding a Fraction to a Number returns a Number (which is a superclass of Fraction).

Maybe also connected to #1237?

@JukkaL
Copy link
Contributor

JukkaL commented Feb 29, 2016

Hmm... mypy may be confused by the type variable in an operator method signature. Anyway, mypy is probably too eager in the overlapping check, as it guards against a pretty theoretical source of unsafety. I'll need to look into the code to figure out what's going on.

Other comments: Should UptoComplexT include Fraction as the first value?

@youtux
Copy link
Contributor Author

youtux commented Mar 1, 2016

Other comments: Should UptoComplexT include Fraction as the first value?

Probably yes, but I preferred to keep the numbers hierarchy (numbers.Rational, numbers.Real and numbers.Complex). I can insert Fraction as first value, too.

However, I think that the most accurate solution would be to have two types:

ForwardOpNumberT = TypeVar('ForwardOpNumberT', int, Fraction, float, complex)
ReverseOpNumberT = TypeVar('ReverseOpNumberT', numbers.Rational, numbers.Real, numbers.Complex)

for the forward and reverse operations like __add__, __radd__, __sub__, __rsub__, etc.

Should I just go with @overloads instead and spare the headache?

@JukkaL
Copy link
Contributor

JukkaL commented Mar 1, 2016

If @overload works, I'd recommend using it and maybe leaving a comment about potentially changing the code to use type variables in the future.

@youtux
Copy link
Contributor Author

youtux commented Mar 1, 2016

Tried with the following:

class Fraction(numbers.Rational):
    ...
    @overload
    def __add__(self, other: Union[int, Fraction]) -> Fraction: ...
    @overload
    def __add__(self, other: float) -> float: ...
    @overload
    def __add__(self, other: complex) -> complex: ...

    @overload
    def __radd__(self, other: numbers.Rational) -> Fraction: ...
    @overload
    def __radd__(self, other: numbers.Real) -> float: ...
    @overload
    def __radd__(self, other: numbers.Complex) -> complex: ...

Mypy still complains:

$ ./runtests.py -x tornado sqlalchemy 3/enum requests
running mypy  # with 207 files
stdlib/2and3/fractions.pyi: note: In class "Fraction":
stdlib/2and3/fractions.pyi:33: error: Signature of "__add__" incompatible with supertype "Complex"
stdlib/2and3/fractions.pyi: note: In member "__radd__" of class "Fraction":
stdlib/2and3/fractions.pyi:41: error: Signatures of "__radd__" of "Fraction" and "__add__" of "Rational" are unsafely overlapping
stdlib/2and3/fractions.pyi: note: In class "Fraction":
stdlib/2and3/fractions.pyi:41: error: Overloaded function signatures 1 and 2 overlap with incompatible return types
stdlib/2and3/fractions.pyi:41: error: Overloaded function signatures 1 and 3 overlap with incompatible return types
stdlib/2and3/fractions.pyi: note: In member "__radd__" of class "Fraction":
stdlib/2and3/fractions.pyi:43: error: Signatures of "__radd__" of "Fraction" and "__add__" of "Real" are unsafely overlapping
stdlib/2and3/fractions.pyi: note: In class "Fraction":
stdlib/2and3/fractions.pyi:43: error: Overloaded function signatures 2 and 2 overlap with incompatible return types
stdlib/2and3/fractions.pyi: note: In member "__radd__" of class "Fraction":
stdlib/2and3/fractions.pyi:45: error: Signatures of "__radd__" of "Fraction" and "__add__" of "Complex" are unsafely overlapping
[mypy --py2 omitted]

Therefore no, @overload does not work.

@JukkaL
Copy link
Contributor

JukkaL commented Mar 1, 2016

I looked in to this. This simplified fragment reproduces the issue:

from typing import TypeVar, Any

class Complex:
    def __add__(self, other): ...
    def __radd__(self, other): ...

class Rational(Complex):
    pass

UpToComplexT = TypeVar('UpToComplexT', Rational, Complex)

class Fraction(Rational):
    def __add__(self, other: UpToComplexT) -> UpToComplexT: ...
    def __radd__(self, other: UpToComplexT) -> UpToComplexT: ...

Mypy is complaining about this because the base classes don't have function annotations, though it perhaps shouldn't complain. However, simply adding annotations may not be sufficient. I'll see if I can give these reasonable types without generating errors.

@gvanrossum
Copy link
Member

@JukkaL Is there a bug in the mypy tracker for this yet? If so could you link to it? If not could you create one? (I feel out of my depth here myself.)

@JukkaL
Copy link
Contributor

JukkaL commented Mar 2, 2016

Because of the current overlapping checks mypy probably can't represent this without giving errors. We'll probably have to make the overlapping checks more lenient, but I'm not sure what's the best way to do it yet. Operator overloading is a tricky area and requires careful thought. I'll create an issue for this in the mypy issue tracker.

@JukkaL
Copy link
Contributor

JukkaL commented Mar 2, 2016

Create issue python/mypy#1264.

@gvanrossum
Copy link
Member

Closed in favor of #544. (@youtux, your attempts were not in vain! And feel free to help out improving the type signatures for #544.)

@gvanrossum gvanrossum closed this Sep 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants