[c++] Pre-neatens for polymorphic domainish accessors (#3011)

* Makefile helper target * helpful comment * method rename * make format for previous commit * more comments * privatize two more methods
single-cell-data · Sep 18, 2024 · 6f723d4 · 6f723d4
1 parent 29be284
commit 6f723d4
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 62 deletions.
diff --git a/Makefile b/Makefile
@@ -38,7 +38,10 @@ test: ctest
 	pytest apis/python/tests
 
 .PHONY: ctest
-ctest: data
+ctest: data ctest_update
+
+.PHONY: ctest_update
+ctest_update:
 	ctest --test-dir build/libtiledbsoma -C Release --verbose --rerun-failed --output-on-failure
 
 .PHONY: data

diff --git a/apis/python/src/tiledbsoma/_dataframe.py b/apis/python/src/tiledbsoma/_dataframe.py
@@ -820,6 +820,9 @@ def _fill_out_slot_soma_domain(
             )
         slot_domain = slot_domain[0], slot_domain[1]
     elif isinstance(dtype, str):
+        # Core string dims have no extent and no (core) domain.  We return "" here
+        # simply so we can pass libtiledbsoma "" for domain and extent, while it
+        # will (and must) ignore these when creating the TileDB schema.
         slot_domain = "", ""
     elif np.issubdtype(dtype, NPInteger):
         iinfo = np.iinfo(cast(NPInteger, dtype))
@@ -887,6 +890,9 @@ def _find_extent_for_domain(
     if isinstance(dtype, np.dtype) and dtype.itemsize == 1:
         extent = 1
 
+    # Core string dims have no extent and no (core) domain.  We return "" here
+    # simply so we can pass libtiledbsoma "" for domain and extent, while it
+    # will (and must) ignore these when creating the TileDB schema.
     if isinstance(dtype, str):
         return ""
 

diff --git a/libtiledbsoma/src/soma/soma_array.h b/libtiledbsoma/src/soma/soma_array.h
@@ -726,9 +726,10 @@ class SOMAArray : public SOMAObject {
     }
 
     /**
-     * Exposed for testing purposes.
+     * Exposed for testing purposes within this library.
+     * Not for use by Python/R.
      */
-    CurrentDomain get_current_domain() const {
+    CurrentDomain get_current_domain_for_test() const {
         return _get_current_domain();
     }
 
@@ -755,9 +756,9 @@ class SOMAArray : public SOMAObject {
     template <typename T>
     std::pair<T, T> soma_domain_slot(const std::string& name) const {
         if (has_current_domain()) {
-            return core_current_domain_slot<T>(name);
+            return _core_current_domain_slot<T>(name);
         } else {
-            return core_domain_slot<T>(name);
+            return _core_domain_slot<T>(name);
         }
     }
 
@@ -771,58 +772,7 @@ class SOMAArray : public SOMAObject {
      */
     template <typename T>
     std::pair<T, T> soma_maxdomain_slot(const std::string& name) const {
-        return core_domain_slot<T>(name);
-    }
-
-    /**
-     * Returns the core current domain at the given dimension.
-     *
-     * o For arrays with core current-domain support:
-     *   - soma domain is core current domain
-     *   - soma maxdomain is core domain
-     * o For arrays without core current-domain support:
-     *   - soma domain is core domain
-     *   - soma maxdomain is core domain
-     *   - core current domain is not accessed at the soma level
-     *
-     * @tparam T Domain datatype
-     * @return Pair of [lower, upper] inclusive bounds.
-     */
-    template <typename T>
-    std::pair<T, T> core_current_domain_slot(const std::string& name) const {
-        CurrentDomain current_domain = _get_current_domain();
-        if (current_domain.is_empty()) {
-            throw TileDBSOMAError(
-                "core_current_domain_slot: internal coding error");
-        }
-        if (current_domain.type() != TILEDB_NDRECTANGLE) {
-            throw TileDBSOMAError(
-                "core_current_domain_slot: found non-rectangle type");
-        }
-        NDRectangle ndrect = current_domain.ndrectangle();
-
-        // Convert from two-element array (core API) to pair (tiledbsoma API)
-        std::array<T, 2> arr = ndrect.range<T>(name);
-        return std::pair<T, T>(arr[0], arr[1]);
-    }
-
-    /**
-     * Returns the core current domain at the given dimension.
-     *
-     * o For arrays with core current-domain support:
-     *   - soma domain is core current domain
-     *   - soma maxdomain is core domain
-     * o For arrays without core current-domain support:
-     *   - soma domain is core domain
-     *   - soma maxdomain is core domain
-     *   - core current domain is not accessed at the soma level
-     *
-     * @tparam T Domain datatype
-     * @return Pair of [lower, upper] inclusive bounds.
-     */
-    template <typename T>
-    std::pair<T, T> core_domain_slot(const std::string& name) const {
-        return arr_->schema().domain().dimension(name).domain<T>();
+        return _core_domain_slot<T>(name);
     }
 
     /**
@@ -945,6 +895,57 @@ class SOMAArray : public SOMAObject {
             *ctx_->tiledb_ctx(), arr_->schema());
     }
 
+    /**
+     * Returns the core current domain at the given dimension.
+     *
+     * o For arrays with core current-domain support:
+     *   - soma domain is core current domain
+     *   - soma maxdomain is core domain
+     * o For arrays without core current-domain support:
+     *   - soma domain is core domain
+     *   - soma maxdomain is core domain
+     *   - core current domain is not accessed at the soma level
+     *
+     * @tparam T Domain datatype
+     * @return Pair of [lower, upper] inclusive bounds.
+     */
+    template <typename T>
+    std::pair<T, T> _core_current_domain_slot(const std::string& name) const {
+        CurrentDomain current_domain = _get_current_domain();
+        if (current_domain.is_empty()) {
+            throw TileDBSOMAError(
+                "_core_current_domain_slot: internal coding error");
+        }
+        if (current_domain.type() != TILEDB_NDRECTANGLE) {
+            throw TileDBSOMAError(
+                "_core_current_domain_slot: found non-rectangle type");
+        }
+        NDRectangle ndrect = current_domain.ndrectangle();
+
+        // Convert from two-element array (core API) to pair (tiledbsoma API)
+        std::array<T, 2> arr = ndrect.range<T>(name);
+        return std::pair<T, T>(arr[0], arr[1]);
+    }
+
+    /**
+     * Returns the core current domain at the given dimension.
+     *
+     * o For arrays with core current-domain support:
+     *   - soma domain is core current domain
+     *   - soma maxdomain is core domain
+     * o For arrays without core current-domain support:
+     *   - soma domain is core domain
+     *   - soma maxdomain is core domain
+     *   - core current domain is not accessed at the soma level
+     *
+     * @tparam T Domain datatype
+     * @return Pair of [lower, upper] inclusive bounds.
+     */
+    template <typename T>
+    std::pair<T, T> _core_domain_slot(const std::string& name) const {
+        return arr_->schema().domain().dimension(name).domain<T>();
+    }
+
     /**
      * Helper method for resize and upgrade_shape.
      */
@@ -963,7 +964,7 @@ class SOMAArray : public SOMAObject {
     void _check_dims_are_int64();
 
     /**
-     * With old shape: core domain mapped to tiledbsoma shape; core current
+     * With old shape: core domain used to map to tiledbsoma shape; core current
      * domain did not exist.
      *
      * With new shape: core domain maps to tiledbsoma maxshape;
@@ -1270,6 +1271,16 @@ class SOMAArray : public SOMAObject {
     uint64_t _nnz_slow();
 };
 
+// These are all specializations to string/bool of various methods
+// which require special handling for that type.
+//
+// Declaring them down here is a bit weird -- they're easy to miss
+// on a read-through. However, we're in a bit of a bind regarding
+// various compilers: if we do these specializations within the
+// `class SOMAArray { ... }`, then one compiler errors if we do
+// include `template <>`, while another errors if we don't.
+// Doing it down here, no compiler complains.
+
 template <>
 void SOMAArray::_cast_dictionary_values<std::string>(
     ArrowSchema* schema, ArrowArray* array);

diff --git a/libtiledbsoma/test/unit_soma_dataframe.cc b/libtiledbsoma/test/unit_soma_dataframe.cc
@@ -501,7 +501,8 @@ TEST_CASE_METHOD(
         // Check current domain
         auto soma_dataframe = open(OpenMode::read);
 
-        CurrentDomain current_domain = soma_dataframe->get_current_domain();
+        CurrentDomain current_domain = soma_dataframe
+                                           ->get_current_domain_for_test();
         if (!use_current_domain) {
             REQUIRE(current_domain.is_empty());
         } else {
@@ -602,7 +603,8 @@ TEST_CASE_METHOD(
         // Check current domain
         auto soma_dataframe = open(OpenMode::read);
 
-        CurrentDomain current_domain = soma_dataframe->get_current_domain();
+        CurrentDomain current_domain = soma_dataframe
+                                           ->get_current_domain_for_test();
         if (!use_current_domain) {
             REQUIRE(current_domain.is_empty());
         } else {
@@ -715,7 +717,8 @@ TEST_CASE_METHOD(
         // Check current domain
         auto soma_dataframe = open(OpenMode::read);
 
-        CurrentDomain current_domain = soma_dataframe->get_current_domain();
+        CurrentDomain current_domain = soma_dataframe
+                                           ->get_current_domain_for_test();
         if (!use_current_domain) {
             REQUIRE(current_domain.is_empty());
         } else {
@@ -831,7 +834,8 @@ TEST_CASE_METHOD(
         // Check current domain
         auto soma_dataframe = open(OpenMode::read);
 
-        CurrentDomain current_domain = soma_dataframe->get_current_domain();
+        CurrentDomain current_domain = soma_dataframe
+                                           ->get_current_domain_for_test();
         if (!use_current_domain) {
             REQUIRE(current_domain.is_empty());
         } else {