TryFrom<Robj> for Int, Logical, Real

[brendanrbrown]

Closes #252

[clauswilke]

Not sure how this relates to #251 after most recent conversations. We need to figure out some coherent approach to handling numeric vectors that does appropriate type conversions and checking, like we are doing with scalars.

[brendanrbrown]

Not sure how this relates to #251 after most recent conversations.

Do you mean #252, which this closes? Or is the suggestion that the NA handling issue should be addressed before this one?

We need to figure out some coherent approach to handling numeric vectors that does appropriate type conversions and checking, like we are doing with scalars.

Agreed. I see this PR as plugging a hole for now as with #247, using the behavior of similar implementations. But of course it should be changed as part of a move to better conversions in general.

[clauswilke]

I meant #251, and specifically, I meant we need to have a coherent framework of how these things should work before continuing to implement.

@Ilia-Kosenkov suggested we should write a whitepaper outlining our data conversion model. This suggestion is on discord. I'll file an issue now here.

[clauswilke]

See #259.

[yutannihilation]

I see this PR as plugging a hole for now as with #247, using the behavior of similar implementations.

Yeah, I thought the same at the time of adding "good first issue" label. Sorry for the confusion...

[clauswilke]

How about something like the following. Instead of this conversion:

robj.as_integer_iter()
            .ok_or_else(|| Error::ExpectedInteger(robj))

could we implement something like this:

if let Ok(v) = robj.as_integer_iter() {
    Ok(v)
} else {
    let rreal = robj.as_real_iter().ok_or_else(|| Error::ExpectedReal(robj))
    let rint = Int::from_real(rreal);
    Ok(rint)
}

where Int::from_real() takes a Real and converts it into an Int, making a copy of the data in the process.

[andy-thomason]

We could implement a "Cow" for numeric types.

A Cow<[type]> is either a slice or a vector depending on whether
a conversion has been done.

So Cow<[i32]> is as_integer_slice for and INTSXP and
a vector conversion for a REALSXP.

So Cow<[f64]> is as_real_slice for and REALSXP and
a vector conversion for a INTSXP.

[andy-thomason]

I may re-work Int, Logical and Real to be just Robj wrappers and pull
out the iterator as a separate object.

We can implement GET_REGION for ALTREP objects which
would be faster.

[andy-thomason]

But essentially, the logic will be the same.

[andy-thomason]

I'm happy to approve this for now if @clauswilke is ok so that @brendanrbrown gets a contribution.

We can either implement Cow types for int/real mismatches or improve the iterators
to accept Altrep types and do the conversions as a separate step.

[clauswilke]

Before merging, can we at least stub out an alternative path for now so we have a marker in the code that this isn't completed?

Something like so maybe?

if let Ok(v) = robj.as_integer_iter() {
    Ok(v)
} else {
    // TODO: check if robj is another type (e.g. Real) that can be converted
    // into integer, and do the conversion
    Error::ExpectedInteger(robj)
}

Also, @andy-thomason, I'd suggest we wait with reworking how Int, Real, and Logical work until we have a bit more progress on an architecture whitepaper, #259. Let's think through all the problems first and then pick an implementation that is appropriate. To me, the question of whether we need types that can stand in for R's equivalent of i32 and f64 is unresolved. (We have something like it for Logical with Bool.)

[brendanrbrown]

I added the todo comment for possible merging, should that be what makes sense here.

could we implement something like this ... where Int::from_real() takes a Real and converts it into an Int

Yes, this seems reasonable, would not be difficult, and would be a step closer to being consistent (probably) with whatever future coherent conversion scheme, based on the conversation. If having it would make this hole-patching PR a better fit, I would be glad to add it.

We could implement a "Cow" for numeric types.

Cow does seem a good choice for the as_ conversions, for exactly the kind of 'maybe integer' conversions discussed above.

I'm happy to approve this for now if @clauswilke is ok so that @brendanrbrown gets a contribution.

@andy-thomason: thanks for the thoughtfulness.

[brendanrbrown]

Don't understand the CI failure here. Since the most recent passing tests I only moved text, but the isn't cargo-fmt related. Someone who knows more able to shed some light?

[clauswilke]

Don't understand the CI failure here.

It may be a probabilistic failure (i.e., works most of the time but breaks sometimes, randomly). I'll restart the tests to see if that fixes it.

[clauswilke]

If having it would make this hole-patching PR a better fit, I would be glad to add it.

If you don't mind I'd appreciate it. We could also break it into two separate PRs though.

I have a few more documentation requests, though:

• Could you add the same kind of TODO comment to the implementation for Real?
• In the test function, could you add a TODO comment that we will have to add tests for reals that can be converted to integers, reals that can't, integers converted to reals, and vectors containing NAs when reals get converted to ints and vice versa?

Thanks!

[brendanrbrown]

@clauswilke My preference would be to address the additional conversions, e.g. using an Int::from_real method, in a separate PR. It'll be easier to review and comment on I think. Should be able to submit an initial PR for that this week.

[clauswilke]

Sounds good. Any idea why the one test is failing?

[brendanrbrown]

It was the same failure as the previous case, in one of the windows builds, which appears inconsistently. I'm not quite sure what to make of it.

[clauswilke]

Thanks!