-
Notifications
You must be signed in to change notification settings - Fork 41
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
More try_from conversions #249
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! I have a few comments and questions, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing that I don't fully understand yet: Shouldn't the contents of the vectors/slices also be converted with try_from()
? It looks to me like we're applying different rules for scalars and vectors/slices right now, e.g. auto-conversion of floats to integers and back.
|
||
// Note the Vec<> cases use the same logic as the slices. | ||
assert_eq!(<&[i32]>::try_from(Robj::from(1.0)), Err(Error::ExpectedInteger(r!(1.0)))); | ||
assert_eq!(<&[f64]>::try_from(Robj::from(1)), Err(Error::ExpectedReal(r!(1)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. Shouldn't this succeed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The slices are a bit of an exception as they give immutable reference access to the interiors of the R objects.
You can still check for NA using value.is_na()
but checking all the values of a gigabyte-long vector may be undesirable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider having mutable slices also &mut [i32]
etc.
This would allow the modification of R objects in-place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I get the argument for the slices, but not for vectors. There we're making a copy of the data, correct? So then we should use the same conversion and rules that we're using for scalars (convert 1.0 to 1 etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The slices are a bit of an exception as they give immutable reference access to the interiors of the R objects.
So, this is also the reason why no other int/float variants (e.g. i64 and f32) are supported in slice's case, right? I'll document the rule on #244.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decided to approve, as this PR moves the project forward.
However, the issue of numerical vectors is not fully resolved, as R doesn't do a good job of distinguishing between integer and floating point values. We need some sort of option that allows a Rust function to accept either integer or float vectors as input and use them as either floats or integers, with auto-conversion as appropriate.
I agree. I think we might need to introduce some wrapper like |
Co-authored-by: Hiroaki Yutani <yutani.ini@gmail.com>
This PR covers i32, f64, Bool and u8 types in vectors and slices.