Skip to content
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.

Sign up for GitHub

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

[mypyc] Implement float abs primitive #9695

Merged
merged 4 commits into from
Nov 4, 2020
Merged

[mypyc] Implement float abs primitive #9695

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c8bcb4
float abs primitive
TH3CHARLie Nov 3, 2020
ea8ad13
add testcase
TH3CHARLie Nov 4, 2020
32f408d
make sure testcase is compiled
TH3CHARLie Nov 4, 2020
72bb9bd
include a positive case in test
TH3CHARLie Nov 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mypyc/test-data/fixtures/ir.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ def zip(x: Iterable[T], y: Iterable[S]) -> Iterator[Tuple[T, S]]: ...
@overload
def zip(x: Iterable[T], y: Iterable[S], z: Iterable[V]) -> Iterator[Tuple[T, S, V]]: ...
def eval(e: str) -> Any: ...
def abs(x: float) -> float: ...

# Dummy definitions.
class classmethod: pass
Expand Down
8 changes: 8 additions & 0 deletions mypyc/test-data/run-floats.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,11 @@ assert str_to_float("1.234567") == 1.234567
assert str_to_float("44324") == 44324.0
assert str_to_float("23.4") == 23.4
assert str_to_float("-43.44e-4") == -43.44e-4

[case testFloatAbs]
[file driver.py]
assert abs(0) == 0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm the result of abs(0) should be of type int. Can you check that the return value is not a float here? You may need to add a similar primitive for abs(integer).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line is incorrect, it should be abs(0.0) == 0.0 while the test will pass because it gets interpreted and 0 == 0.0 returns True. It should be fixed now.

I am not getting what do you mean by adding a primitive for integer? Do you mean a fixture function or a mypyc primitive?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean a primitive for abs(5), where the argument is an integer (in int_ops.py). It can use PyNumber_Absolute for now, similar to the float op, but the return type would be int instead of float.

I think that we may currently use the float variant of the primitive for integer arguments, which doesn't seem right. More generally, it's better for consistency to also cover the int case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. I've verified that in the above case, abs(0) returns an int by checking the generated code and running the compiled module. So we wouldn't be using float variant for integer arguments.

The integer version primitive would better be implemented in a separate PR as it requires casts from CPyTagged to PyObject * if we are going to reuse PyNumber_Absolute

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing this in a separate PR sounds good. I was worried that we might use the float op with an int argument since int is compatible with float. Thanks for checking that this isn't a real issue.

Implementing the integer variant in IR build would be even better, since it would avoid boxing/unboxing. Can you create a follow-up issue about this (or PR)? However, using PyNumber_Absolute would be a reasonable intermediate step.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are on the same track. By building the op in irbuild, we can have a fast path for short ints.

assert abs(-1.234567) == 1.234567
assert abs(44324) == 44324.0
assert abs(-23.4) == 23.4
assert abs(-43.44e-4) == 43.44e-4
TH3CHARLie marked this conversation as resolved.
Show resolved Hide resolved