polish based on comments in 0602237

Author: LynxJinyangii

RcppTskit/NEWS.md

@@ -75,6 +75,21 @@ and releases adhere to [Semantic Versioning](https://semver.org/spec/v2.0.0.html
   `pointer` to `xptr`.
 - Ensured `TableCollection$tree_sequence()` matches `tskit Python` API:
   it now builds indexes on the `TableCollection`, if indexes are not present.
+- Expanded `TableCollection$*_table_get_row()` documentation with runnable
+  examples, Python `__getitem__` references, and explicit note that numeric
+  row indices are validated and converted to 32-bit integers.
+- Aligned low-level row getters with `tskit C` caveats by returning
+  `mutations = NULL` for `rtsk_site_table_get_row()`, exposing `edge` and
+  `inherited_state` (as `NULL` for table access) in
+  `rtsk_mutation_table_get_row()`, and exposing `nodes` in
+  `rtsk_individual_table_get_row()`.
+- Kept high-level `TableCollection$individual_table_get_row()`,
+  `TableCollection$site_table_get_row()`, and
+  `TableCollection$mutation_table_get_row()` aligned with `tskit Python`
+  row-shape semantics.
+- Refined integer validators to use a single 32-bit integer-like validation
+  path with explicit `strict` handling for integer-only versus numeric-allowed
+  inputs across scalar and optional vector arguments.
 - We now use `bit64::integer64` (signed 64 bit integer) instead of `int` aiming
   to approach `tsk_size_t` in `tskit C` (unsigned 64 bit integer); in low-level
   `rtsk_treeseq_get_num_*()` wrappers and count/metadata-length fields.

RcppTskit/R/Class-TableCollection.R

@@ -138,7 +138,7 @@ TableCollection <- R6Class(
       rtsk_table_collection_get_num_provenances(self$xptr)
     },
 
-    #' @description Add a row to the provenances table.
+    #' @description Add a row to the provenance table.
     #' @param timestamp character string timestamp for the new provenance.
     #' @param record character string record for the new provenance.
     #' @details See the \code{tskit Python} equivalent at
@@ -163,10 +163,18 @@ TableCollection <- R6Class(
       )
     },
 
-    #' @description Get one row from the provenances table.
-    #' @param index integer scalar row index (0-based).
+    #' @description Get one row from the provenance table.
+    #' @param index integer or numeric scalar row index (0-based).
+    #' @details See the \code{tskit Python} equivalent at
+    #'   \url{https://tskit.dev/tskit/docs/stable/python-api.html#tskit.ProvenanceTable.__getitem__}.
+    #'   The function accepts numeric \code{index} for ease of use, but converts
+    #'   it to integer after checking that conversion to 32-bit integer succeeds.
     #' @return A named list with fields \code{id}, \code{timestamp},
     #'   and \code{record}.
+    #' @examples
+    #' ts_file <- system.file("examples/test.trees", package = "RcppTskit")
+    #' tc <- tc_load(ts_file)
+    #' tc$provenance_table_get_row(0)
     provenance_table_get_row = function(index) {
       validate_row_index(index)
       rtsk_provenance_table_get_row(self$xptr, index = as.integer(index))
@@ -204,9 +212,17 @@ TableCollection <- R6Class(
       )
     },
 
-    #' @description Get one row from the populations table.
-    #' @param index integer scalar row index (0-based).
+    #' @description Get one row from the population table.
+    #' @param index integer or numeric scalar row index (0-based).
+    #' @details See the \code{tskit Python} equivalent at
+    #'   \url{https://tskit.dev/tskit/docs/stable/python-api.html#tskit.PopulationTable.__getitem__}.
+    #'   The function accepts numeric \code{index} for ease of use, but converts
+    #'   it to integer after checking that conversion to 32-bit integer succeeds.
     #' @return A named list with fields \code{id} and \code{metadata}.
+    #' @examples
+    #' ts_file <- system.file("examples/test.trees", package = "RcppTskit")
+    #' tc <- tc_load(ts_file)
+    #' tc$population_table_get_row(0)
     population_table_get_row = function(index) {
       validate_row_index(index)
       rtsk_population_table_get_row(self$xptr, index = as.integer(index))
@@ -287,11 +303,24 @@ TableCollection <- R6Class(
       )
     },
 
-    #' @description Get one row from the migrations table.
-    #' @param index integer scalar row index (0-based).
+    #' @description Get one row from the migration table.
+    #' @param index integer or numeric scalar row index (0-based).
+    #' @details See the \code{tskit Python} equivalent at
+    #'   \url{https://tskit.dev/tskit/docs/stable/python-api.html#tskit.MigrationTable.__getitem__}.
+    #'   The function accepts numeric \code{index} for ease of use, but converts
+    #'   it to integer after checking that conversion to 32-bit integer succeeds.
     #' @return A named list with fields \code{id}, \code{left}, \code{right},
     #'   \code{node}, \code{source}, \code{dest}, \code{time}, and
     #'   \code{metadata}.
+    #' @examples
+    #' ts_file <- system.file("examples/test.trees", package = "RcppTskit")
+    #' tc <- tc_load(ts_file)
+    #' if (as.integer(tc$num_migrations()) == 0L) {
+    #'   tc$migration_table_add_row(
+    #'     left = 0, right = 1, node = 0L, source = 0L, dest = 0L, time = 1
+    #'   )
+    #' }
+    #' tc$migration_table_get_row(0)
     migration_table_get_row = function(index) {
       validate_row_index(index)
       rtsk_migration_table_get_row(self$xptr, index = as.integer(index))
@@ -348,13 +377,22 @@ TableCollection <- R6Class(
       )
     },
 
-    #' @description Get one row from the individuals table.
-    #' @param index integer scalar row index (0-based).
+    #' @description Get one row from the individual table.
+    #' @param index integer or numeric scalar row index (0-based).
+    #' @details See the \code{tskit Python} equivalent at
+    #'   \url{https://tskit.dev/tskit/docs/stable/python-api.html#tskit.IndividualTable.__getitem__}.
+    #'   The function accepts numeric \code{index} for ease of use, but converts
+    #'   it to integer after checking that conversion to 32-bit integer succeeds.
     #' @return A named list with fields \code{id}, \code{flags},
     #'   \code{location}, \code{parents}, and \code{metadata}.
+    #' @examples
+    #' ts_file <- system.file("examples/test.trees", package = "RcppTskit")
+    #' tc <- tc_load(ts_file)
+    #' tc$individual_table_get_row(0)
     individual_table_get_row = function(index) {
       validate_row_index(index)
-      rtsk_individual_table_get_row(self$xptr, index = as.integer(index))
+      row <- rtsk_individual_table_get_row(self$xptr, index = as.integer(index))
+      row[c("id", "flags", "location", "parents", "metadata")]
     },
 
     #' @description Get the number of nodes in a table collection.
@@ -432,11 +470,13 @@ TableCollection <- R6Class(
     # TODO: Similarly with add_row method on the R side, maybe?
     # TODO: ALSO, should we use 0-based or 1-based access to elements of an object!? I think not!?
     #       And should we allow characters as ID names?
-    #' @description Get one row from the nodes table.
-    #' @param index integer scalar row index (0-based).
+    #' @description Get one row from the node table.
+    #' @param index integer or numeric scalar row index (0-based).
     #' @details In \code{tskit Python}, rows are accessed by indexing a
     #'   \code{NodeTable}, for example \code{tables.nodes[index]}; see
     #'   \url{https://tskit.dev/tskit/docs/stable/python-api.html#tskit.NodeTable}.
+    #'   The function accepts numeric \code{index} for ease of use, but converts
+    #'   it to integer after checking that conversion to 32-bit integer succeeds.
     #' @return A named list with fields \code{id}, \code{flags}, \code{time},
     #'   \code{population}, \code{individual}, and \code{metadata}.
     #' @examples
@@ -510,10 +550,18 @@ TableCollection <- R6Class(
       )
     },
 
-    #' @description Get one row from the edges table.
-    #' @param index integer scalar row index (0-based).
+    #' @description Get one row from the edge table.
+    #' @param index integer or numeric scalar row index (0-based).
+    #' @details See the \code{tskit Python} equivalent at
+    #'   \url{https://tskit.dev/tskit/docs/stable/python-api.html#tskit.EdgeTable.__getitem__}.
+    #'   The function accepts numeric \code{index} for ease of use, but converts
+    #'   it to integer after checking that conversion to 32-bit integer succeeds.
     #' @return A named list with fields \code{id}, \code{left}, \code{right},
     #'   \code{parent}, \code{child}, and \code{metadata}.
+    #' @examples
+    #' ts_file <- system.file("examples/test.trees", package = "RcppTskit")
+    #' tc <- tc_load(ts_file)
+    #' tc$edge_table_get_row(0)
     edge_table_get_row = function(index) {
       validate_row_index(index)
       rtsk_edge_table_get_row(self$xptr, index = as.integer(index))
@@ -560,13 +608,22 @@ TableCollection <- R6Class(
       )
     },
 
-    #' @description Get one row from the sites table.
-    #' @param index integer scalar row index (0-based).
+    #' @description Get one row from the site table.
+    #' @param index integer or numeric scalar row index (0-based).
+    #' @details See the \code{tskit Python} equivalent at
+    #'   \url{https://tskit.dev/tskit/docs/stable/python-api.html#tskit.SiteTable.__getitem__}.
+    #'   The function accepts numeric \code{index} for ease of use, but converts
+    #'   it to integer after checking that conversion to 32-bit integer succeeds.
     #' @return A named list with fields \code{id}, \code{position},
     #'   \code{ancestral_state}, and \code{metadata}.
+    #' @examples
+    #' ts_file <- system.file("examples/test.trees", package = "RcppTskit")
+    #' tc <- tc_load(ts_file)
+    #' tc$site_table_get_row(0)
     site_table_get_row = function(index) {
       validate_row_index(index)
-      rtsk_site_table_get_row(self$xptr, index = as.integer(index))
+      row <- rtsk_site_table_get_row(self$xptr, index = as.integer(index))
+      row[c("id", "position", "ancestral_state", "metadata")]
     },
 
     #' @description Get the number of mutations in a table collection.
@@ -645,14 +702,31 @@ TableCollection <- R6Class(
       )
     },
 
-    #' @description Get one row from the mutations table.
-    #' @param index integer scalar row index (0-based).
+    #' @description Get one row from the mutation table.
+    #' @param index integer or numeric scalar row index (0-based).
+    #' @details See the \code{tskit Python} equivalent at
+    #'   \url{https://tskit.dev/tskit/docs/stable/python-api.html#tskit.MutationTable.__getitem__}.
+    #'   The function accepts numeric \code{index} for ease of use, but converts
+    #'   it to integer after checking that conversion to 32-bit integer succeeds.
     #' @return A named list with fields \code{id}, \code{site}, \code{node},
-    #'   \code{parent}, \code{time}, \code{derived_state}, and
-    #'   \code{metadata}.
+    #'   \code{derived_state}, \code{parent}, \code{metadata}, and
+    #'   \code{time}.
+    #' @examples
+    #' ts_file <- system.file("examples/test.trees", package = "RcppTskit")
+    #' tc <- tc_load(ts_file)
+    #' tc$mutation_table_get_row(0)
     mutation_table_get_row = function(index) {
       validate_row_index(index)
-      rtsk_mutation_table_get_row(self$xptr, index = as.integer(index))
+      row <- rtsk_mutation_table_get_row(self$xptr, index = as.integer(index))
+      row[c(
+        "id",
+        "site",
+        "node",
+        "derived_state",
+        "parent",
+        "metadata",
+        "time"
+      )]
     },
 
     #' @description Get the sequence length.

RcppTskit/R/RcppTskit.R

@@ -127,45 +127,56 @@ validate_integer_scalar_arg <- function(
   allow_null = FALSE,
   strict = FALSE
 ) {
-  int_min <- -as.numeric(.Machine$integer.max) - 1
-  int_max <- .Machine$integer.max
-
   if (is.null(value)) {
     if (allow_null) {
       return(NULL)
     }
     stop(name, " cannot be NULL.", call. = FALSE)
   }
 
-  is_type_ok <- if (strict) is.integer(value) else is.numeric(value)
-  is_valid_scalar <- is_type_ok &&
-    length(value) == 1L &&
-    !is.na(value) &&
-    is.finite(value) &&
-    value == trunc(value)
-
-  in_range <- is_valid_scalar &&
-    value >= int_min &&
-    value <= int_max &&
-    (is.null(minimum) || value >= minimum)
-
-  if (!in_range) {
-    reqs <- if (identical(minimum, 0L)) {
-      "a non-NA zero or positive integer scalar within 32-bit range"
-    } else {
-      "a non-NA integer scalar within 32-bit range"
-    }
-    if (!is.null(minimum) && !identical(minimum, 0L)) {
-      reqs <- paste0(reqs, " (>= ", minimum, ")")
+  abort <- function() {
+    msg <- "a non-NA integer scalar within 32-bit range"
+    if (identical(minimum, 0L)) {
+      msg <- "a non-NA zero or positive integer scalar within 32-bit range"
+    } else if (!is.null(minimum)) {
+      msg <- paste0(msg, " (>= ", minimum, ")")
     }
     if (allow_null) {
-      reqs <- paste("NULL or", reqs)
+      stop(name, " must be NULL or ", msg, "!", call. = FALSE)
+    }
+    stop(name, " must be ", msg, "!", call. = FALSE)
+  }
+
+  is_valid_type <- if (strict) {
+    is.integer(value)
+  } else {
+    is.numeric(value)
+  }
+  if (!is_valid_type || length(value) != 1L || is.na(value)) {
+    abort()
+  }
+
+  if (is.integer(value)) {
+    if (!is.null(minimum) && value < minimum) {
+      abort()
     }
+    return(value)
+  }
+
+  value_num <- as.numeric(value)
+  int_min <- -as.numeric(.Machine$integer.max) - 1
+  int_max <- as.numeric(.Machine$integer.max)
+  is_whole <- value_num %% 1 == 0
+  is_within_bounds <- value_num >= int_min && value_num <= int_max
+  is_above_min <- is.null(minimum) || value_num >= minimum
 
-    stop(name, " must be ", reqs, "!", call. = FALSE)
+  if (
+    !is.finite(value_num) || !is_whole || !is_within_bounds || !is_above_min
+  ) {
+    abort()
   }
 
-  as.integer(value)
+  return(as.integer(value_num))
 }
 
 # INTERNAL
@@ -207,30 +218,37 @@ validate_optional_numeric_vector_arg <- function(value, name) {
 #   unless \code{strict = TRUE} (in that case this function throws an error).
 # @return No return value; called for side effects.
 validate_optional_integer_vector_arg <- function(value, name, strict = FALSE) {
-  int_min <- -(.Machine$integer.max) - 1
-  int_max <- .Machine$integer.max
+  int_min <- -as.numeric(.Machine$integer.max) - 1
+  int_max <- as.numeric(.Machine$integer.max)
+
   if (is.null(value)) {
     return(invisible(NULL))
   }
+
   if (strict && !is.integer(value)) {
     stop(
       name,
       " must be NULL or an integer vector with no NA values within 32-bit range!"
     )
   }
+
+  value_num <- suppressWarnings(as.numeric(value))
+
   if (
     !is.numeric(value) ||
-      anyNA(value) ||
-      !all(is.finite(value)) ||
-      any(value != trunc(value)) ||
-      any(value < int_min) ||
-      any(value > int_max)
+      anyNA(value_num) ||
+      !all(is.finite(value_num)) ||
+      any(value_num != trunc(value_num)) ||
+      any(value_num < int_min) ||
+      any(value_num > int_max)
   ) {
     stop(
       name,
       " must be NULL or an integer vector with no NA values within 32-bit range!"
     )
   }
+
+  invisible(as.integer(value_num))
 }
 
 # INTERNAL