Skip to content

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

Merged
certik merged 13 commits into
lcompilers:mainfrom
czgdp1807:manual_casting
Nov 9, 2022
Merged

Disable implicit casting by default and implement intrinsic functions for casting#1271
certik merged 13 commits into
lcompilers:mainfrom
czgdp1807:manual_casting

Conversation

@czgdp1807

Copy link
Copy Markdown
Collaborator

Overrides #1194

Comment thread integration_tests/array_expr_01.py Outdated

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
Copy Markdown
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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed f64.

Comment thread integration_tests/elemental_03.py Outdated
# 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread tests/reference/asr-expr7-480ba2f.json Outdated
"stderr_hash": "28509dd59a386eebd632340a550d14299cd2a921ef6dc3ac7dbe7fe9",
"returncode": 0
"stderr_hash": "d862bc9fbb666cb9925f68e2e3494851ee5d21f55e629078df671b83",
"returncode": 2

Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread src/runtime/lpython_intrinsic_numpy.py Outdated
@vectorize
def radians(x: f32) -> f32:
return x*pi_32/180
return x*f32(pi_32/f32(180.0))

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread integration_tests/const_03.py

@certik certik left a comment

Copy link
Copy Markdown
Contributor

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