Altrep Integers class. #274

andy-thomason · 2021-08-30T16:21:00Z

This is a sample for an Integers class that uses Altrep
methods to handle very large vectors.

There are no iterators at present as we would need to implement
Rint first.

The USP of this is the ability to make vectors from iterators without
allocating memory.

We support elt(), get_region(), is_sorted() and no_na() correctly.

We do not have access to altrep min() and max() since these were removed
from Rinternals.h

Suggestions on a postcard...

clauswilke · 2021-08-30T16:42:06Z

Thanks for your efforts! Two comments:

What about the non-altrep variant? I believe the idea was that Integers would be an opaque wrapper that can represent both altrep and regular vectors. @Ilia-Kosenkov suggested AltIntegers for an altrep-only wrapper for integers: https://github.com/extendr/extendr/blob/03d06173fd457c338bfe371a9d9f211c44f32051/marshaling_rules.md#naming-convention So maybe implement AltIntegers first and then use that to implement Integers?
If Rint is needed for feature completeness should that be implemented first or at the same time? I assume it's relatively lightweight.

andy-thomason · 2021-08-31T09:28:52Z

This should handle both the ALTREP and regular vectors.

The methods we call like get_region and elt should also work
for regular vectors.

When constructing vectors, I'm chosing to use regular vectors
for small ones and ALTVEC for larger ones. This is because there
is some overhead in creating the class and vector and we
need to generate the class every time.

The regular conversion rules should accept anything with INTSXP
as input. The macro in mod.rs converts from Robj to Integers and
vice versa.

extendr-api/src/wrapper/mod.rs

clauswilke · 2021-08-31T14:01:54Z

The methods we call like get_region and elt should also work
for regular vectors.

I see, thanks!

extendr-api/src/scalar/rint.rs

clauswilke · 2021-08-31T22:22:28Z

The Rint prototype looks good to me. We'll probably want to add TryFrom traits eventually, but it's a good starting point.

yutannihilation

Thanks, this pull request is a really nice starting point for discussion.

extendr-api/src/wrapper/integers.rs

clauswilke · 2021-09-04T17:23:20Z

extendr-api/src/wrapper/integers.rs

+    ///     assert_eq!(vec.iter().cloned().sum::<i32>(), 3);
+    /// }
+    /// ```
+    pub fn iter(&self) -> impl Iterator<Item = &i32> {


Apologies for being nitpicky: Shouldn't the item type be Rint? In the general case, we don't know that we can return an i32 for sure (could be NA).

I thought this would return SliceIter<T>. Or, is the idea that doing NA-validation beforehand at once, which is similar to this comment above? If so, i32 might make sense.

do all its checks once for a number of elements

Good point...

I've implemented Rint where possible.

I'm in two minds about get_region as we would like to support SIMD arithmetic but can do this too if required.

The SliceIter should probably go. Its only useful feature is that the lifetime
is effectively static.

yutannihilation · 2021-09-05T02:10:35Z

extendr-api/src/scalar/rint.rs

+
+impl std::iter::Sum for Rint {
+    /// Sum an integer iterator over Rint.
+    /// Yields NA on overflow of NAs present.


Yields NA on overflow of NAs present.

If this sentence means to say no NA-validation is needed because of this, I don't think it's true. (e.g. c(-1, NA))

Ah, I see. So, anyway we need NA-validation here, right?

Okay, I thought this would return i32::MIX - 1, but I found this is handled correctly thanks to <Rint>::checked_sub(). Sorry for my misunderstanding!

rextendr::rust_source(code = r"( use extendr_api::scalar::Rint; #[extendr(use_try_from = true)] fn do_sum(x: Integers) -> Rint { x.iter().sum() } )", patch.crates_io = list(`extendr-api` = list(git = "https://github.com/extendr/extendr")) ) #> ℹ build directory: '/tmp/Rtmp7RG5fJ/file77e25279e1bf0' #> ✓ Writing '/tmp/Rtmp7RG5fJ/file77e25279e1bf0/target/extendr_wrappers.R'. do_sum(c(NA_integer_, -1L)) #> [1] NA

^{Created on 2021-09-12 by the reprex package (v2.0.1)}

yutannihilation · 2021-09-05T02:11:39Z

extendr-api/src/scalar/rint.rs

+        // is problematic when converting a negative value to unsigned
+        // integer types (e.g. `-1i32 as u8` becomes 255).
+        if let Some(v) = robj.as_integer() {
+            if let Ok(v) = Self::try_from(v) {


I think we can skip this try_from in this case. Rint is 32-bit, so it should never overflow.

extendr-api/src/wrapper/integers.rs

yutannihilation · 2021-09-06T14:18:33Z

extendr-api/src/wrapper/integers.rs

+    /// Get a single element from the vector.
+    /// Note that this is very inefficient in a tight loop.
+    pub fn elt(&self, index: usize) -> Rint {
+        unsafe { INTEGER_ELT(self.get(), index as R_xlen_t).into() }


In the case of non-ALTREP, cpp11 accesses to the underlying memory directly without INTEGER_ELT. Can we do the same thing? Doesn't this matter much on the performance?

https://github.com/r-lib/cpp11/blob/1b4beb25f56c8dca6e0448d5dc55fb64fddab5d1/inst/include/cpp11/integers.hpp#L29-L32

yutannihilation · 2021-09-06T14:19:31Z

extendr-api/src/wrapper/integers.rs

+        unsafe { INTEGER_IS_SORTED(self.get()).into() }
+    }
+
+    /// Return TRUE if the vector has NAs, FALSE if not, or NA_BOOL if unknown.


Suggested change

/// Return TRUE if the vector has NAs, FALSE if not, or NA_BOOL if unknown.

/// Return TRUE if the vector has no NAs, FALSE if not, or NA_BOOL if unknown.

andy-thomason · 2021-09-06T16:37:33Z

Just so you know that we all very much respect your views.

…

On Mon, Sep 6, 2021 at 3:31 PM Hiroaki Yutani ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In extendr-api/src/wrapper/integers.rs <#274 (comment)>: > + } else { + let mut robj = Robj::alloc_vector(INTSXP, values.len()); + let dest: &mut [i32] = robj.as_typed_slice_mut().unwrap(); + + for (d, v) in dest.iter_mut().zip(values) { + *d = v.into(); + } + robj + }; + Self { robj } + }) + } + + /// Get a single element from the vector. + /// Note that this is very inefficient in a tight loop. + pub fn elt(&self, index: usize) -> Rint { I don't have a strong opinion. I think I'll be fine with Rint. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#274 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAL36XAZG23WTWIIGBKSUW3UATGCXANCNFSM5DCG5Q2Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

yutannihilation · 2021-09-07T14:20:16Z

Thanks, I just feel I don't have any idea worth arguing about there for now :) Let's move forward.

Altrep Integers class.

e23f1b4

andy-thomason commented Aug 31, 2021

View reviewed changes

extendr-api/src/wrapper/mod.rs Show resolved Hide resolved

Prototype Rint, a scalar integer type.

97d5c8f

clauswilke reviewed Aug 31, 2021

View reviewed changes

extendr-api/src/scalar/rint.rs Show resolved Hide resolved

clauswilke reviewed Aug 31, 2021

View reviewed changes

extendr-api/src/scalar/rint.rs Show resolved Hide resolved

yutannihilation reviewed Sep 1, 2021

View reviewed changes

extendr-api/src/wrapper/integers.rs Outdated Show resolved Hide resolved

Implement FromIterator, iter() and iter_mut()

463683b

clauswilke reviewed Sep 4, 2021

View reviewed changes

andy-thomason added 2 commits September 4, 2021 20:32

Rint for iterators and elt.

c5cfd80

More conversions and tests.

6841e6e

yutannihilation reviewed Sep 5, 2021

View reviewed changes

yutannihilation reviewed Sep 6, 2021

View reviewed changes

andy-thomason mentioned this pull request Sep 9, 2021

Implement "Doubles" wrapper #280

Closed

andy-thomason merged commit 7e9cac3 into master Sep 9, 2021

andy-thomason deleted the Integers-prototype-for-discussion branch September 9, 2021 12:59

This was referenced Sep 23, 2021

Int, Real conversions follow-up #264

Closed

SliceIter<T>::from_slice, SliceIter<T>::next allow unsafe behavior #267

Closed

multimeric mentioned this pull request May 28, 2022

Add proper wrappers for vector types and make names consistent and intuitive #266

Open

Ilia-Kosenkov mentioned this pull request May 29, 2022

Rewrite changelog for 0.3.0 release #410

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Altrep Integers class. #274

Altrep Integers class. #274

andy-thomason commented Aug 30, 2021

clauswilke commented Aug 30, 2021

andy-thomason commented Aug 31, 2021

clauswilke commented Aug 31, 2021

clauswilke commented Aug 31, 2021

yutannihilation left a comment

clauswilke Sep 4, 2021

yutannihilation Sep 4, 2021

andy-thomason Sep 4, 2021

andy-thomason Sep 4, 2021

andy-thomason Sep 4, 2021

yutannihilation Sep 5, 2021

andy-thomason Sep 6, 2021

yutannihilation Sep 6, 2021

yutannihilation Sep 12, 2021

yutannihilation Sep 5, 2021

andy-thomason Sep 6, 2021

yutannihilation Sep 6, 2021

yutannihilation Sep 6, 2021

andy-thomason commented Sep 6, 2021 via email

yutannihilation commented Sep 7, 2021

	/// Return TRUE if the vector has NAs, FALSE if not, or NA_BOOL if unknown.
	/// Return TRUE if the vector has no NAs, FALSE if not, or NA_BOOL if unknown.

Altrep Integers class. #274

Altrep Integers class. #274

Conversation

andy-thomason commented Aug 30, 2021

clauswilke commented Aug 30, 2021

andy-thomason commented Aug 31, 2021

clauswilke commented Aug 31, 2021

clauswilke commented Aug 31, 2021

yutannihilation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-thomason commented Sep 6, 2021 via email

yutannihilation commented Sep 7, 2021