Skip to content

Commit

Permalink
[python/r] Remove 2.27-related feature flag (#3368)
Browse files Browse the repository at this point in the history
* [python/r] Remove 2.27-related feature flag

* more

* [c++] Some `use_current_domain` unit-test/feature-flag teardown, part 1 of 4 (#3369)

* [c++] Some `use_current_domain` teardown

* lint
  • Loading branch information
johnkerl authored Nov 22, 2024
1 parent 4d7bff2 commit 8461b0a
Show file tree
Hide file tree
Showing 17 changed files with 101 additions and 262 deletions.
64 changes: 23 additions & 41 deletions apis/python/src/tiledbsoma/_dense_nd_array.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
from ._arrow_types import pyarrow_to_carrow_type
from ._common_nd_array import NDArray
from ._exception import SOMAError, map_exception_for_create
from ._flags import DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN
from ._tdb_handles import DenseNDArrayWrapper
from ._types import OpenTimestamp, Slice, StatusAndReason
from ._util import dense_indices_to_shape
Expand Down Expand Up @@ -122,37 +121,26 @@ def create(
if dim_shape is None:
raise ValueError("DenseNDArray shape slots must be numeric")

if DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
dim_capacity, dim_extent = cls._dim_capacity_and_extent(
dim_name,
# The user specifies current domain -- this is the max domain
# which is taken from the max ranges for the dim datatype.
# We pass None here to detect those.
None,
ndim,
TileDBCreateOptions.from_platform_config(platform_config),
)

if dim_shape == 0:
raise ValueError("DenseNDArray shape slots must be at least 1")

index_column_data[pa_field.name] = [
0,
dim_capacity - 1,
dim_extent,
0,
dim_shape - 1,
]
dim_capacity, dim_extent = cls._dim_capacity_and_extent(
dim_name,
# The user specifies current domain -- this is the max domain
# which is taken from the max ranges for the dim datatype.
# We pass None here to detect those.
None,
ndim,
TileDBCreateOptions.from_platform_config(platform_config),
)

else:
dim_capacity, dim_extent = cls._dim_capacity_and_extent(
dim_name,
dim_shape,
ndim,
TileDBCreateOptions.from_platform_config(platform_config),
)
if dim_shape == 0:
raise ValueError("DenseNDArray shape slots must be at least 1")

index_column_data[pa_field.name] = [0, dim_capacity - 1, dim_extent]
index_column_data[pa_field.name] = [
0,
dim_capacity - 1,
dim_extent,
0,
dim_shape - 1,
]

index_column_info = pa.RecordBatch.from_pydict(
index_column_data, schema=pa.schema(index_column_schema)
Expand Down Expand Up @@ -356,10 +344,7 @@ def resize(self, newshape: Sequence[Union[int, None]]) -> None:
"""Supported for ``SparseNDArray``; scheduled for implementation for
``DenseNDArray`` in TileDB-SOMA 1.15
"""
if DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
self._handle.resize(newshape)
else:
raise NotImplementedError("Not implemented for libtiledbsoma < 2.27.0")
self._handle.resize(newshape)

def tiledbsoma_upgrade_shape(
self, newshape: Sequence[Union[int, None]], check_only: bool = False
Expand All @@ -368,14 +353,11 @@ def tiledbsoma_upgrade_shape(
1.15 release notes. Raises an error if the new shape exceeds maxshape in
any dimension. Raises an error if the array already has a shape.
"""
if DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
if check_only:
return self._handle.tiledbsoma_can_upgrade_shape(newshape)
else:
self._handle.tiledbsoma_upgrade_shape(newshape)
return (True, "")
if check_only:
return self._handle.tiledbsoma_can_upgrade_shape(newshape)
else:
raise NotImplementedError("Not implemented for libtiledbsoma < 2.27.0")
self._handle.tiledbsoma_upgrade_shape(newshape)
return (True, "")

@classmethod
def _dim_capacity_and_extent(
Expand Down
21 changes: 0 additions & 21 deletions apis/python/src/tiledbsoma/_flags.py

This file was deleted.

25 changes: 6 additions & 19 deletions apis/python/src/tiledbsoma/_tdb_handles.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@

from . import pytiledbsoma as clib
from ._exception import DoesNotExistError, SOMAError, is_does_not_exist_error
from ._flags import DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN
from ._types import METADATA_TYPES, Metadatum, OpenTimestamp, StatusAndReason
from .options._soma_tiledb_context import SOMATileDBContext

Expand Down Expand Up @@ -630,37 +629,25 @@ def tiledbsoma_has_upgraded_shape(self) -> bool:

def resize(self, newshape: Sequence[Union[int, None]]) -> None:
"""Wrapper-class internals"""
if DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
self._handle.resize(newshape)
else:
raise NotImplementedError("Not implemented for libtiledbsoma < 2.27.0")
self._handle.resize(newshape)

def tiledbsoma_can_resize(
self, newshape: Sequence[Union[int, None]]
) -> StatusAndReason:
"""Wrapper-class internals"""
if DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
return cast(StatusAndReason, self._handle.tiledbsoma_can_resize(newshape))
else:
raise NotImplementedError("Not implemented for libtiledbsoma < 2.27.0")
return cast(StatusAndReason, self._handle.tiledbsoma_can_resize(newshape))

def tiledbsoma_upgrade_shape(self, newshape: Sequence[Union[int, None]]) -> None:
"""Wrapper-class internals"""
if DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
self._handle.tiledbsoma_upgrade_shape(newshape)
else:
raise NotImplementedError("Not implemented for libtiledbsoma < 2.27.0")
self._handle.tiledbsoma_upgrade_shape(newshape)

def tiledbsoma_can_upgrade_shape(
self, newshape: Sequence[Union[int, None]]
) -> StatusAndReason:
"""Wrapper-class internals"""
if DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
return cast(
StatusAndReason, self._handle.tiledbsoma_can_upgrade_shape(newshape)
)
else:
raise NotImplementedError("Not implemented for libtiledbsoma < 2.27.0")
return cast(
StatusAndReason, self._handle.tiledbsoma_can_upgrade_shape(newshape)
)


class SparseNDArrayWrapper(SOMAArrayWrapper[clib.SOMASparseNDArray]):
Expand Down
16 changes: 5 additions & 11 deletions apis/python/tests/test_dense_nd_array.py
Original file line number Diff line number Diff line change
Expand Up @@ -410,17 +410,11 @@ def test_tile_extents(tmp_path):

with soma.DenseNDArray.open(tmp_path.as_posix()) as A:
dim_info = json.loads(A.config_options_from_schema().dims)
if soma._flags.DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
# With new-shape, core current domain is (100,10000) but core domain
# is huge, and therefore dim 0 does not get its extent squashed down
# to 100.
assert int(dim_info["soma_dim_0"]["tile"]) == 512
assert int(dim_info["soma_dim_1"]["tile"]) == 512
else:
# This assumes core domain is (100,10000) and therefore dim 0
# gets its extent squashed down to 100.
assert int(dim_info["soma_dim_0"]["tile"]) == 100
assert int(dim_info["soma_dim_1"]["tile"]) == 512
# With new shape (tiledbsoma 1.15), core current domain is (100,10000)
# but core domain is huge, and therefore dim 0 does not get its extent
# squashed down to 100.
assert int(dim_info["soma_dim_0"]["tile"]) == 512
assert int(dim_info["soma_dim_1"]["tile"]) == 512


def test_timestamped_ops(tmp_path):
Expand Down
35 changes: 12 additions & 23 deletions apis/python/tests/test_shape.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,37 +188,26 @@ def test_dense_nd_array_basics(tmp_path):

with tiledbsoma.DenseNDArray.open(uri) as dnda:
assert dnda.shape == (100, 200)
if tiledbsoma._flags.DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
assert len(dnda.maxshape)
assert dnda.maxshape[0] > 2**62
assert dnda.maxshape[1] > 2**62
else:
assert dnda.maxshape == (100, 200)
assert len(dnda.maxshape)
assert dnda.maxshape[0] > 2**62
assert dnda.maxshape[1] > 2**62

assert dnda.non_empty_domain() == ((0, 0), (0, 0))

with tiledbsoma.DenseNDArray.open(uri, "w") as dnda:
if tiledbsoma._flags.DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
dnda.resize((300, 400))
else:
with pytest.raises(NotImplementedError):
dnda.resize((300, 400))
dnda.resize((300, 400))

with tiledbsoma.DenseNDArray.open(uri) as dnda:
assert dnda.non_empty_domain() == ((0, 0), (0, 0))
if tiledbsoma._flags.DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
assert dnda.shape == (300, 400)
else:
assert dnda.shape == (100, 200)
assert dnda.shape == (300, 400)

if tiledbsoma._flags.DENSE_ARRAYS_CAN_HAVE_CURRENT_DOMAIN:
with tiledbsoma.DenseNDArray.open(uri) as dnda:
ok, msg = dnda.tiledbsoma_upgrade_shape((600, 700), check_only=True)
assert not ok
assert (
msg
== "tiledbsoma_can_upgrade_shape: array already has a shape: please use resize"
)
with tiledbsoma.DenseNDArray.open(uri) as dnda:
ok, msg = dnda.tiledbsoma_upgrade_shape((600, 700), check_only=True)
assert not ok
assert (
msg
== "tiledbsoma_can_upgrade_shape: array already has a shape: please use resize"
)


@pytest.mark.parametrize(
Expand Down
4 changes: 0 additions & 4 deletions apis/r/R/SOMANDArrayBase.R
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,6 @@ SOMANDArrayBase <- R6::R6Class(
#' @return No return value
resize = function(new_shape) {
stopifnot(
"resize is not supported for dense arrays until tiledbsoma 1.15" =
.dense_arrays_can_have_current_domain() || private$.is_sparse,
"'new_shape' must be a vector of integerish values, of the same length as maxshape" =
rlang::is_integerish(new_shape, n = self$ndim()) ||
(bit64::is.integer64(new_shape) && length(new_shape) == self$ndim())
Expand All @@ -125,8 +123,6 @@ SOMANDArrayBase <- R6::R6Class(
#' @return No return value
tiledbsoma_upgrade_shape = function(shape) {
stopifnot(
"tiledbsoma_upgrade_shape is not supported for dense arrays until tiledbsoma 1.15" =
.dense_arrays_can_have_current_domain() || private$.is_sparse,
"'shape' must be a vector of integerish values, of the same length as maxshape" =
rlang::is_integerish(shape, n = self$ndim()) ||
(bit64::is.integer64(shape) && length(shape) == self$ndim())
Expand Down
6 changes: 1 addition & 5 deletions apis/r/R/utils-arrow.R
Original file line number Diff line number Diff line change
Expand Up @@ -577,11 +577,7 @@ get_domain_and_extent_array <- function(shape, is_sparse) {
# expansion.
ind_max_dom <- arrow_type_unsigned_range(ind_col_type) - c(0, ind_ext)

if (is_sparse || .dense_arrays_can_have_current_domain()) {
aa <- arrow::arrow_array(c(ind_max_dom, ind_ext, ind_cur_dom), ind_col_type)
} else {
aa <- arrow::arrow_array(c(ind_cur_dom, ind_ext), ind_col_type)
}
aa <- arrow::arrow_array(c(ind_max_dom, ind_ext, ind_cur_dom), ind_col_type)

aa
})
Expand Down
21 changes: 0 additions & 21 deletions apis/r/R/zzz.R
Original file line number Diff line number Diff line change
Expand Up @@ -19,27 +19,6 @@
)
packageStartupMessage(msg)
}

# This is temporary for https://github.com/single-cell-data/TileDB-SOMA/issues/2407
# It will be removed once 2407 is complete.
if (Sys.getenv("SOMA_R_NEW_SHAPE") == "false") {
.pkgenv[["use_current_domain_transitional_internal_only"]] <- FALSE
} else {
.pkgenv[["use_current_domain_transitional_internal_only"]] <- TRUE
}
}

# Temporary for # https://github.com/single-cell-data/TileDB-SOMA/issues/2407.
# Once core 2.27 is released and we depend on it, this can go away.
.dense_arrays_can_have_current_domain <- function() {
# This allows testing dense + current domain on the same machine without
# having to switch core builds (or switch machines).
if (Sys.getenv("SOMA_IGNORE_CORE_2_27") != "") {
return(FALSE)
}

triple <- tiledb_embedded_version()
return(triple[[1]] >= 2 && triple[[2]] >= 27)
}

## An .onAttach() function is not allowed to use cat() etc but _must_ communicate via
Expand Down
28 changes: 4 additions & 24 deletions apis/r/tests/testthat/test-11-shape.R
Original file line number Diff line number Diff line change
Expand Up @@ -520,15 +520,7 @@ test_that("SOMADenseNDArray shape", {
readback_maxshape <- ndarray$maxshape()
expect_equal(length(readback_shape), length(readback_maxshape))

if (.dense_arrays_can_have_current_domain()) {
expect_true(all(readback_shape < readback_maxshape))
} else {
expect_true(all(readback_shape == readback_maxshape))
}

if (! tiledbsoma:::.dense_arrays_can_have_current_domain()) {
expect_equal(readback_shape, readback_maxshape)
}
expect_true(all(readback_shape < readback_maxshape))

ndarray$close()

Expand Down Expand Up @@ -565,29 +557,17 @@ test_that("SOMADenseNDArray shape", {

# Test resize up
new_shape <- c(500, 600)
if (tiledbsoma:::.dense_arrays_can_have_current_domain()) {
expect_no_error(ndarray$resize(new_shape))
} else {
expect_error(ndarray$resize(new_shape))
}
expect_no_error(ndarray$resize(new_shape))

# Test writes within new bounds
ndarray <- SOMADenseNDArrayOpen(uri, "WRITE")
mat <- create_dense_matrix_with_int_dims(500, 600)
if (tiledbsoma:::.dense_arrays_can_have_current_domain()) {
expect_no_error(ndarray$write(mat))
} else {
expect_error(ndarray$write(mat))
}
expect_no_error(ndarray$write(mat))
ndarray$close()

ndarray <- SOMADenseNDArrayOpen(uri)
coords <- list(bit64::as.integer64(c(101, 202)), bit64::as.integer64(c(3, 4)))
if (tiledbsoma:::.dense_arrays_can_have_current_domain()) {
expect_no_condition(x <- ndarray$read_dense_matrix(coords = coords))
} else {
expect_error(x <- ndarray$read(coords = coords)$tables()$concat())
}
expect_no_condition(x <- ndarray$read_dense_matrix(coords = coords))
ndarray$close()

rm(ndarray)
Expand Down
8 changes: 2 additions & 6 deletions libtiledbsoma/src/utils/arrow_adapter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1188,13 +1188,9 @@ std::pair<Dimension, bool> ArrowAdapter::tiledb_dimension_from_arrow_schema(
FilterList filter_list = ArrowAdapter::_create_dim_filter_list(
col_name, platform_config, soma_type, ctx);

if (array->length == 3) {
use_current_domain = false;
} else if (array->length == 5) {
// This is fine
} else {
if (array->length != 5) {
throw TileDBSOMAError(std::format(
"ArrowAdapter: unexpected length {} for name "
"ArrowAdapter: unexpected length {} != 5 for name "
"{}",
array->length,
col_name));
Expand Down
Loading

0 comments on commit 8461b0a

Please sign in to comment.