Skip to content

Disable implicit casting by default and implement intrinsic functions for casting #1271

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

Merged
merged 13 commits into from
Nov 9, 2022

Conversation

czgdp1807
Copy link
Collaborator

Overrides #1194

@czgdp1807 czgdp1807 force-pushed the manual_casting branch 3 times, most recently from ac7a16b to 72fbba8 Compare November 8, 2022 14:27
@czgdp1807 czgdp1807 marked this pull request as ready for review November 8, 2022 14:27
@czgdp1807 czgdp1807 requested a review from certik November 8, 2022 14:27
@@ -36,6 +36,6 @@ def array_expr_01():
g = reshape(e + f, shape1d)

for i in range(dim1d):
assert abs(g[i] - 2*(i + 1)) <= eps
assert f64(abs(g[i] - f64(2*(i + 1)))) <= eps
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the outer f64 is not necessary? I think abs of f64 should return an f64.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed f64.

# for l in range(256):
# i = i32(l/16)
# j = (l - i*16)
# assert abs(observed[l]**f32(2.0) - f32(i + j)) <= eps
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these commented out?

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 forgot to uncomment them after debugging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

"stderr_hash": "28509dd59a386eebd632340a550d14299cd2a921ef6dc3ac7dbe7fe9",
"returncode": 0
"stderr_hash": "d862bc9fbb666cb9925f68e2e3494851ee5d21f55e629078df671b83",
"returncode": 2
Copy link
Contributor

Choose a reason for hiding this comment

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

test now returns an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


@overload
@vectorize
def radians(x: f32) -> f32:
return x*pi_32/180
return x*f32(pi_32/f32(180.0))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be written just as:

x*pi_32/f32(180)

? Or will that not work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually / operator always returns f64 type. This is because I think CPython always returns float with / operator. float is equivalent to f64 so we need to manually cast it to f32.

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 was happening implicitly but now got visibility when implicit casting is turned off.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Yes, Python returns float for /.

But we currently do f32 * f32 -> f32, that is, multiplication keeps the kind of the floating point. So why not also do f32 / f32 -> f32 and make the division keep the kind?

In Python it is simply float * float -> float and float / float -> float.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. I am +1 on this.

--> tests/errors/kwargs_02_error.py:9:5
|
9 | arg3 = 3.0
| ^^^^ ^^^ type mismatch ('f32' and 'f64')
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message has changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in b5b4c82

4 | c.real = 5.0
| ^^^^^^^^^^^^
3 | c = complex(3, 4)
| ^ ^^^^^^^^^^^^^ type mismatch ('c32' and 'c64')
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a different error? I think you need to cast it first, so that you get to the actual error that we want to test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in b5b4c82

8 | print(c1 | c2)
| ^^^^^^^
5 | c1 = 4+5j
| ^ ^^ type mismatch (i32 and c64)
Copy link
Contributor

Choose a reason for hiding this comment

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

different error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in b5b4c82

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this is good enough. The rest we can fix later as we discover issues.

Thanks @czgdp1807 for this monumental work. Very helpful! This provides us with a solid base. We can always implement inference or implicit rules later without breaking backwards compatibility.

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.

2 participants