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.

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

DM-29117: Allow dict interface to upgrade integer types #64

Merged
merged 4 commits into from Mar 12, 2021

Conversation

timj
Copy link
Member

@timj timj commented Mar 10, 2021

They are not downgraded so an Int64 will stay an Int64 even if assigned "42" but an Int32 will upgrade to an Int64 if needed.

@timj timj requested a review from ktlim March 10, 2021 22:44
@ktlim
Copy link
Contributor

ktlim commented Mar 10, 2021

On second thought, how does the handling of existing types mesh with add()? I think it will cause problems.

@timj
Copy link
Member Author

timj commented Mar 10, 2021

I was wondering about array items, especially since we already only look at the first element in an array. I'll add a test but surely it would be broken already so may simply be broken differently after this.

@ktlim
Copy link
Contributor

ktlim commented Mar 10, 2021

Looks like it just changes the exception from an out-of-range to a mismatched-type. And adding smaller values to larger types should work correctly.

@timj
Copy link
Member Author

timj commented Mar 10, 2021

It issues TypeError if you add a large number to an existing small int type. They do come from different parts of the pybind11 interface. That doesn't seem fixable (and is not part of the dict interface anyhow).

One thing that is fixable is dealing with initializing with [1, LARGE] vs [LARGE, 1] int arrays. We'd have to scan the entire array to look for the range of integers.

@timj
Copy link
Member Author

timj commented Mar 11, 2021

[1, LARGE] should now work.

Previously [1, LARGE] failed because we only looked at the first
value to determine the range.
and also add some tests.
@timj timj merged commit fc31b0f into master Mar 12, 2021
@timj timj deleted the tickets/DM-29117 branch March 12, 2021 14:39
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.

None yet

2 participants