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

Implement Logicals #338

Merged
merged 6 commits into from
Dec 13, 2021
Merged

Implement Logicals #338

merged 6 commits into from
Dec 13, 2021

Conversation

andy-thomason
Copy link
Contributor

This PR does the following:

Add Rbool as a type - Bool is still there but will be deprecated.
Add Logicals as a type.
Redesign the gen_vector_wrapper_impl macro a little, but keeping Ilia's original design.
Make sure everything works with scalar types including Integers and Doubles.
Implement ALTREP logicals support.
Add Logicals to Rany.
Fix many many tests.

@andy-thomason andy-thomason linked an issue Dec 8, 2021 that may be closed by this pull request
@Ilia-Kosenkov
Copy link
Member

What about a few trivial tests on R side here https://github.com/extendr/extendr/blob/master/tests/extendrtests/src/rust/src/lib.rs?
E.g., test for NA and elt access, similar to Integer and Doubles?

@andy-thomason
Copy link
Contributor Author

@clauswilke Tell me if I should drop Bool in this PR?

@clauswilke
Copy link
Member

I think dropping Bool now is fine. The current version of extendr is basically a complete rewrite of the previous version anyways, so I don't see any reason to keep backwards compatibility.

@andy-thomason
Copy link
Contributor Author

Good. I'll do this then.

@andy-thomason
Copy link
Contributor Author

We may need to review the current case that:

NA_LOGICAL != NA_LOGICAL

Because PartialEq never returns true for NAs on Rbool, Rint or Rfloat.

@Ilia-Kosenkov
Copy link
Member

We may need to review the current case that:

NA_LOGICAL != NA_LOGICAL

Because PartialEq never returns true for NAs on Rbool, Rint or Rfloat.

Can you explain this? It seems that if PartialEq never returns true for NA, then NA_LOGICAL != NA_LOGICAL, or am I missing something?

@clauswilke
Copy link
Member

On the R side, you cannot compare an NA to an NA. The result is neither TRUE nor FALSE but rather NA. So returning TRUE when comparing NA to NA would definitely be problematic.

@Ilia-Kosenkov
Copy link
Member

I am aware of this feature of R. Why can't we treat all NAs as floating-point NaNs, i.e. any equality operation involving an NA results in false, even NA == NA. This is somewhat consistent with R implementation, as I doubt anyone treats logical NA obtained during comparison as TRUE.

@clauswilke
Copy link
Member

I agree, that's the most reasonable choice. It does imply NA_LOGICAL != NA_LOGICAL, though. Maybe I misunderstood what you were trying to say earlier. I though you considered it a problem.

@Ilia-Kosenkov
Copy link
Member

I was trying to say NA != NA is the approach I prefer. Sorry for confusion.

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.

Logicals
3 participants