fix: panic on negative exponent in ipow #758
Conversation
ss2165
requested review from
mark-koch and
croyzor
and removed request for
mark-koch
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #758 +/- ##
==========================================
- Coverage 92.97% 92.94% -0.03%
==========================================
Files 71 71
Lines 8222 8224 +2
==========================================
Hits 7644 7644
- Misses 578 580 +2 ☔ View full report in Codecov by Sentry. |
guppylang/std/builtins.py
Outdated
| @@ -362,7 +362,8 @@ def __pos__(self: int) -> int: ... | |||
| @no_type_check | |||
| def __pow__(self: int, other: int) -> int: | |||
| # only positive exponents are supported | |||
| # TODO add panic for negative exponents once #756 is resolved | |||
| if other < 0: | |||
| panic("Negative exponent not supported in __pow__.") | |||
There was a problem hiding this comment.
Not supported for now? Or does guppy disallow it? I think using negative exponents to take roots is the kind of freaky thing physicists would do so I'm inclined to say we should support it. It is very awkward however, the right behaviour is like:
def __pow(self: int, other: int) -> int | float:
if other < 0:
return float(self) ** float(other)
else:
# do the ipow thing, return an int
but I think the float | int return type is problematic? I tried to do it quickly and I'm not quite sure what goes wrong
There was a problem hiding this comment.
guppy does not support unions and probably never will, we will support algebraic data types at some point but it is unclear whether we can make them behave like unions (especially with regards to type inference).
There was a problem hiding this comment.
Could add a message saying "cast to float first" though?
There was a problem hiding this comment.
That would work - make sure to say it's the base that needs cast to a float
There was a problem hiding this comment.
Casting the exponent should also work due to __rpow__
There was a problem hiding this comment.
Casting the exponent should also work due to
__rpow__
Nice, I didn't realise!
There was a problem hiding this comment.
Does that work as written here, or does it require __pow__ to use ReversingChecker?
There was a problem hiding this comment.
Should work as written. The ReversingChecker is on __rpow__
No description provided.