Fixing bugs in cxg conversion tool (#1782)

server/cli/cli.py

@@ -1,9 +1,10 @@
 import click
 
-from .. import __version__
+from .convert_to_cxg import convert_to_cxg
 from .launch import launch
 from .prepare import prepare
 from .upgrade import log_upgrade_check
+from .. import __version__
 
 
 @click.group(
@@ -29,3 +30,4 @@ def cli(upgrade_check):
 
 cli.add_command(launch)
 cli.add_command(prepare)
+cli.add_command(convert_to_cxg)

server/cli/convert_to_cxg.py

@@ -16,12 +16,11 @@ from server.converters.h5ad_data_file import H5ADDataFile
 @click.argument(
     "input-file",
     nargs=1,
-    help="Path to the H5AD input file to be converted.",
     type=click.Path(exists=True, dir_okay=False),
 )
 @click.option(
     "-o",
-    "--output-dir",
+    "--output-directory",
     help="Name of the output CXG directory. If not provided, will default to be the input filename with a "
          "CXG extension.",
 )
@@ -70,9 +69,9 @@ from server.converters.h5ad_data_file import H5ADDataFile
 )
 @click.option(
     "--disable-corpora-schema",
-    "When set, conversion process will neither extract nor store Corpora schema information. See "
-    "https://github.com/chanzuckerberg/corpora-data-portal/blob/main/backend/schema/corpora_schema.md for "
-    "more information.",
+    help="When set, conversion process will neither extract nor store Corpora schema information. See "
+         "https://github.com/chanzuckerberg/corpora-data-portal/blob/main/backend/schema/corpora_schema.md for more "
+         "information.",
     default=False,
     show_default=True,
     is_flag=True,
@@ -84,7 +83,6 @@ from server.converters.h5ad_data_file import H5ADDataFile
     show_default=True,
     is_flag=True,
 )
-@click.option("-v", "--verbose", count=True)
 @click.help_option("--help", "-h", help="Show this message and exit.")
 def convert_to_cxg(
         input_file,
@@ -97,7 +95,7 @@ def convert_to_cxg(
         var_names,
         disable_custom_colors,
         disable_corpora_schema,
-        should_overwrite,
+        overwrite,
 ):
     """
     Convert a dataset file into CXG.
@@ -107,7 +105,7 @@ def convert_to_cxg(
                                   use_corpora_schema=not disable_corpora_schema)
 
     # Get the directory that will hold all the CXG files
-    cxg_output_container = get_output_directory(input_file, output_directory, should_overwrite)
+    cxg_output_container = get_output_directory(input_file, output_directory, overwrite)
 
     h5ad_data_file.to_cxg(cxg_output_container, sparse_threshold,
                           convert_anndata_colors_to_cxg_colors=not disable_custom_colors)
@@ -118,14 +116,14 @@ def get_output_directory(input_filename, output_directory, should_overwrite):
     Get the name of the CXG output directory to be created/populated during the dataset conversion.
     """
 
-    if not path.isdir(output_directory) or (path.isdir(output_directory) and should_overwrite):
+    if output_directory and (not path.isdir(output_directory) or (path.isdir(output_directory) and should_overwrite)):
         if output_directory.endswith(".cxg"):
             return output_directory
         return output_directory + ".cxg"
-    if path.isdir(output_directory) and not should_overwrite:
+    if output_directory and path.isdir(output_directory) and not should_overwrite:
         raise click.BadParameter(
             f"Output directory {output_directory} already exists. If you'd like to overwrite, then run the command "
             f"with the --overwrite flag."
         )
 
-    return path.splitext(input_filename)[1] + ".cxg"
+    return path.splitext(input_filename)[0] + ".cxg"

server/common/utils/cxg_generation_utils.py

@@ -67,7 +67,6 @@ def convert_dataframe_to_cxg_array(cxg_container, dataframe_name, dataframe, ind
         schema_hints = {}
         for column_name, column_values in dataframe.items():
             dtype, hints = get_dtype_and_schema_of_array(column_values)
-
             value[column_name] = column_values.to_numpy(dtype=dtype)
             if hints:
                 schema_hints.update({column_name: hints})

server/common/utils/type_conversion_utils.py

@@ -37,19 +37,19 @@ def get_dtype_from_dtype(dtype, array_values=None):
     dtype_name = dtype.name
     dtype_kind = dtype.kind
 
-    if dtype == np.float32 or dtype == np.int32:
-        return dtype
     if dtype_name == "bool":
         return np.uint8
     if dtype_name == "object" and dtype_kind == "O":
         return np.unicode
     if dtype_name == "category":
-        return get_dtype_from_dtype(dtype.categories.dtype, dtype.categories)
+        return get_dtype_from_dtype(dtype.categories.dtype, array_values)
 
-    if can_cast_to_float32(dtype):
-        return np.float32
     if can_cast_to_int32(dtype, array_values):
         return np.int32
+    if can_cast_to_float32(dtype, array_values):
+        return np.float32
+    if not can_cast_to_float32(dtype, array_values):
+        return np.float64
 
     raise TypeError(f"Annotations of type {dtype} are unsupported.")
 
@@ -72,19 +72,43 @@ def get_schema_type_hint_from_dtype(dtype, array_values=None):
     if dtype_name == "category":
         return {"type": "categorical", "categories": dtype.categories.tolist()}
 
-    if can_cast_to_float32(dtype):
-        return {"type": "float32"}
     if can_cast_to_int32(dtype, array_values):
         return {"type": "int32"}
+    if can_cast_to_float32(dtype, array_values):
+        return {"type": "float32"}
+    if dtype_kind == "f" and not can_cast_to_float32(dtype, array_values):
+        return {"type": "float64"}
 
     raise TypeError(f"Annotations of type {dtype} are unsupported.")
 
 
-def can_cast_to_float32(dtype):
+def can_cast_to_float32(dtype, array_values):
+    """
+    A dtype can be cast to float32 if it is a float type and converting it to float32 presents the same output as the
+    original values. Note that NaNs fail equality (i.e. np.NaN != np.NaN) so we use np.testing.assert_equal to ensure
+    that the arrays are equal minus NaNs.
+
+    We also handle a special case here where the array is a Series object with integer categorical values AND NaNs.
+    Since NaNs are floating points in numpy, we upcast the integer array to float32.
+    """
+
     if dtype.kind == "f":
-        if not np.can_cast(dtype, np.float32):
+        # Try to convert the array to float32
+        converted_float32_values = array_values.to_numpy(np.float32)
+        original_values = array_values.to_numpy()
+
+        # Verify that the two arrays are equal except for NaNs (which will equate to be unequal).
+        if not ((converted_float32_values != original_values) == np.isnan(original_values)).all():
+            return False
+
+        if dtype != np.float32:
             logging.warning(f"Type {dtype.name} will be converted to 32 bit float and may lose precision.")
+
         return True
+
+    if dtype.kind == "O" and array_values.hasnans:
+        return True
+
     return False
 
 
@@ -94,11 +118,30 @@ def can_cast_to_int32(dtype, array_values=None):
     the higher precision type has values that are entirely within the range of the downcast type.
     """
 
+    # Since a NaN is technically a float, any array that contains NaNs cannot be cast to an integer so immediately
+    # return False.
+    if array_values.hasnans:
+        return False
+
+    # If the array is categorical, then we need to order the array values so that functions min and max that occur
+    # later, can function. They do not function on unordered categories.
+    ordered_array_values = array_values
+    if array_values.dtype.name == "category" and not array_values.cat.ordered:
+        ordered_array_values = array_values.cat.as_ordered()
+
     if dtype.kind in ["i", "u"]:
         if np.can_cast(dtype, np.int32):
             return True
         ii32 = np.iinfo(np.int32)
-        if not array_values.empty and (
-                array_values.min() >= ii32.min and array_values.max() <= ii32.max) or array_values.empty:
+        if not ordered_array_values.empty and (
+                ordered_array_values.min() >= ii32.min and ordered_array_values.max() <= ii32.max) or \
+                ordered_array_values.empty:
             return True
     return False
+
+
+def convert_pandas_series_to_numpy(series_to_convert: pd.Series, dtype):
+    if series_to_convert.hasnans and dtype == np.int32:
+        logging.error("Cannot convert a pandas Series object to an integer dtype if it contains NaNs.")
+
+    return series_to_convert.to_numpy(dtype)

server/data_common/fbs/matrix.py

@@ -85,7 +85,7 @@ def serialize_typed_array(builder, source_array, encoding_info):
 def column_encoding(arr):
     column_encoding_type_map = {
         # array protocol string:  ( array_type, as_type )
-        np.dtype(np.float64).str: (TypedArray.TypedArray.Float32Array, np.float32),
+        np.dtype(np.float64).str: (TypedArray.TypedArray.Float64Array, np.float64),
         np.dtype(np.float32).str: (TypedArray.TypedArray.Float32Array, np.float32),
         np.dtype(np.float16).str: (TypedArray.TypedArray.Float32Array, np.float32),
         np.dtype(np.int8).str: (TypedArray.TypedArray.Int32Array, np.int32),

server/data_cxg/cxg_adaptor.py

@@ -1,24 +1,25 @@
-import os
 import json
 import logging
-from server.common.utils.type_conversion_utils import get_schema_type_hint_from_dtype
-from server.common.errors import DatasetAccessError, ConfigurationError
-from server.common.utils.utils import path_join
+import os
+import threading
+
+import numpy as np
+import pandas as pd
+import tiledb
+from server_timing import Timing as ServerTiming
+
+import server.compute.diffexp_cxg as diffexp_cxg
 from server.common.constants import Axis
+from server.common.errors import DatasetAccessError, ConfigurationError
+from server.common.immutable_kvcache import ImmutableKVCache
+from server.common.utils.type_conversion_utils import get_schema_type_hint_from_dtype
+from server.common.utils.utils import path_join
 from server.data_common.data_adaptor import DataAdaptor
 from server.data_common.fbs.matrix import encode_matrix_fbs
 from server.data_cxg.cxg_util import pack_selector_from_mask
-import server.compute.diffexp_cxg as diffexp_cxg
-from server.common.immutable_kvcache import ImmutableKVCache
-import tiledb
-import numpy as np
-import pandas as pd
-from server_timing import Timing as ServerTiming
-import threading
 
 
 class CxgAdaptor(DataAdaptor):
-
     # TODO:  The tiledb context parameters should be a configuration option
     tiledb_ctx = tiledb.Ctx(
         {"sm.tile_cache_size": 8 * 1024 * 1024 * 1024, "sm.num_reader_threads": 32, "vfs.s3.region": "us-east-1"}
@@ -337,32 +338,6 @@ class CxgAdaptor(DataAdaptor):
             raise DatasetAccessError("cxg matrix missing embeddings")
         return embeddings
 
-    @staticmethod
-    def _get_col_type(attr, schema_hints={}):
-        type_hint = schema_hints.get(attr.name, {})
-        dtype = attr.dtype
-        schema = {}
-        # type hints take precedence
-        if "type" in type_hint:
-            schema["type"] = type_hint["type"]
-        elif dtype == np.float32:
-            schema["type"] = "float32"
-        elif dtype == np.int32:
-            schema["type"] = "int32"
-        elif dtype == np.bool_:
-            schema["type"] = "boolean"
-        elif dtype == np.str:
-            schema["type"] = "string"
-        elif dtype == "category":
-            schema["type"] = "categorical"
-            schema["categories"] = dtype.categories.tolist()
-        else:
-            raise TypeError(f"Annotations of type {dtype} are unsupported.")
-
-        if schema["type"] == "categorical" and "categories" in schema_hints:
-            schema["categories"] = schema_hints["categories"]
-        return schema
-
     def _get_schema(self):
         if self.schema:
             return self.schema

server/test/unit/common/utils/test_type_conversion_utils.py

@@ -1,11 +1,12 @@
 import unittest
+from time import time
 from unittest.mock import patch
 
 import numpy as np
 from pandas import Series, DataFrame
 
 from server.common.utils.type_conversion_utils import can_cast_to_float32, can_cast_to_int32, get_dtype_of_array, \
-    get_schema_type_hint_of_array, get_dtypes_and_schemas_of_dataframe
+    get_schema_type_hint_of_array, get_dtypes_and_schemas_of_dataframe, convert_pandas_series_to_numpy
 
 
 class TestTypeConversionUtils(unittest.TestCase):
@@ -13,28 +14,49 @@ class TestTypeConversionUtils(unittest.TestCase):
     def test__can_cast_to_float32__string_is_false(self):
         array_to_convert = Series(data=["1", "2", "3"], dtype=str)
 
-        can_cast = can_cast_to_float32(array_to_convert.dtype)
+        can_cast = can_cast_to_float32(array_to_convert.dtype, array_to_convert)
 
         self.assertFalse(can_cast)
 
-    def test__can_cast_to_float32__int_is_true_warning_outputted(self):
+    def test__can_cast_to_float32__float64_is_true_warning_outputted(self):
         array_to_convert = Series(data=[1, 2, 3], dtype=np.dtype(np.float64))
 
         with self.assertLogs(level="WARN") as logger:
-            can_cast = can_cast_to_float32(array_to_convert.dtype)
+            can_cast = can_cast_to_float32(array_to_convert.dtype, array_to_convert)
             self.assertIn("may lose precision", logger.output[0])
 
         self.assertTrue(can_cast)
 
     @patch("logging.warning")
-    def test__can_cast_to_float64__int_is_false(self, mock_log_warning):
+    def test__can_cast_to_float32__float32_is_false(self, mock_log_warning):
         array_to_convert = Series(data=[1, 2, 3], dtype=np.dtype(np.float32))
 
-        can_cast = can_cast_to_float32(array_to_convert.dtype)
+        can_cast = can_cast_to_float32(array_to_convert.dtype, array_to_convert)
 
         self.assertTrue(can_cast)
         assert not mock_log_warning.called
 
+    def test__can_cast_to_float32__categorical_float64_is_false(self):
+        array_to_convert = Series(data=[1.1, 2.2, 3.3], dtype="category")
+
+        can_cast = can_cast_to_float32(array_to_convert.dtype, array_to_convert)
+
+        self.assertFalse(can_cast)
+
+    def test__can_cast_to_float32__categorical_int64_with_nans_is_true(self):
+        array_to_convert = Series(data=[1, 2, np.NaN], dtype="category")
+
+        can_cast = can_cast_to_float32(array_to_convert.dtype, array_to_convert)
+
+        self.assertTrue(can_cast)
+
+    def test__can_cast_to_float_32__float_32_with_nans_is_true(self):
+        array_to_convert = Series(data=[1, 2, np.NaN], dtype=np.dtype(np.float32))
+
+        can_cast = can_cast_to_float32(array_to_convert.dtype, array_to_convert)
+
+        self.assertTrue(can_cast)
+
     def test__can_cast_to_int32__string_is_false(self):
         array_to_convert = Series(data=["1", "2", "3"], dtype=str)
 
@@ -63,6 +85,13 @@ class TestTypeConversionUtils(unittest.TestCase):
 
         self.assertFalse(can_cast)
 
+    def test__can_cast_to_int32__int64_with_nans_is_false(self):
+        array_to_convert = Series(data=[np.NaN, "2", "3"], dtype="category")
+
+        can_cast = can_cast_to_int32(array_to_convert.dtype, array_to_convert)
+
+        self.assertFalse(can_cast)
+
     def test__get_dtype_of_array__supported_dtypes_return_as_expected(self):
         types = [np.float32, np.int32, np.bool_, str]
         expected_dtypes = [np.float32, np.int32, np.uint8, np.unicode]
@@ -73,6 +102,40 @@ class TestTypeConversionUtils(unittest.TestCase):
                 array = Series(data=[], dtype=types[test_type_index])
                 self.assertEqual(get_dtype_of_array(array), expected_dtypes[test_type_index])
 
+    def test__get_dtype_of_array__categories_return_as_expected(self):
+        array = Series(data=["a", "b", "c"], dtype="category")
+        expected_dtype = np.unicode
+
+        actual_dtype = get_dtype_of_array(array)
+
+        self.assertEqual(expected_dtype, actual_dtype)
+
+    def test__get_dtype_of_array__unordered_integer_categories_return_as_expected(self):
+        array = Series(data=[2, 3, 1, 3, 1, 2], dtype="category")
+        expected_dtype = np.int32
+
+        actual_dtype = get_dtype_of_array(array)
+
+        self.assertEqual(expected_dtype, actual_dtype)
+
+    def test__get_dtype_of_array__castable_dtypes_return_as_expected(self):
+        types = [np.float64, np.int64]
+        expected_dtypes = [np.float32, np.int32]
+
+        for test_type_index in range(len(types)):
+            with self.subTest(f"Testing get_dtype_of_array with castable type {types[test_type_index].__name__}",
+                              i=test_type_index):
+                array = Series(data=[], dtype=types[test_type_index])
+                self.assertEqual(get_dtype_of_array(array), expected_dtypes[test_type_index])
+
+    def test__get_dtype_of_array__unsupported_type_raises_exception(self):
+        unsupported_array = Series(list([time() for _ in range(2)]), dtype="datetime64[ns]")
+
+        with self.assertRaises(TypeError) as exception_context:
+            get_dtype_of_array(unsupported_array)
+
+        self.assertIn("unsupported", str(exception_context.exception))
+
     def test__get_schema_type_hint_of_array__supported_dtypes_return_as_expected(self):
         types = [np.float32, np.int32, np.bool_, str]
         expected_schema_hints = [{"type": "float32"}, {"type": "int32"}, {"type": "boolean"}, {"type": "string"}]
@@ -83,14 +146,6 @@ class TestTypeConversionUtils(unittest.TestCase):
                 array = Series(data=[], dtype=types[test_type_index])
                 self.assertEqual(get_schema_type_hint_of_array(array), expected_schema_hints[test_type_index])
 
-    def test__get_dtype_of_array__categories_return_as_expected(self):
-        array = Series(data=["a", "b", "c"], dtype="category")
-        expected_dtype = np.unicode
-
-        actual_dtype = get_dtype_of_array(array)
-
-        self.assertEqual(expected_dtype, actual_dtype)
-
     def test__get_schema_type_hint_of_array__categories_return_as_expected(self):
         array = Series(data=["a", "b", "b"], dtype="category")
         expected_schema_hint = {"type": "categorical", "categories": ["a", "b"]}
@@ -99,16 +154,6 @@ class TestTypeConversionUtils(unittest.TestCase):
 
         self.assertEqual(expected_schema_hint, actual_schema_hint)
 
-    def test__get_dtype_of_array__castable_dtypes_return_as_expected(self):
-        types = [np.float64, np.int64]
-        expected_dtypes = [np.float32, np.int32]
-
-        for test_type_index in range(len(types)):
-            with self.subTest(f"Testing get_dtype_of_array with castable type {types[test_type_index].__name__}",
-                              i=test_type_index):
-                array = Series(data=[], dtype=types[test_type_index])
-                self.assertEqual(get_dtype_of_array(array), expected_dtypes[test_type_index])
-
     def test__get_schema_type_hint_of_array__castable_dtypes_return_as_expected(self):
         types = [np.float64, np.int64]
         expected_schema_hints = [{"type": "float32"}, {"type": "int32"}]
@@ -133,3 +178,28 @@ class TestTypeConversionUtils(unittest.TestCase):
 
         self.assertEqual(expected_data_types_dict, actual_dataframe_data_types)
         self.assertEqual(expected_schema_type_hints_dict, actual_dataframe_schema_type_hints)
+
+    def test__convert_pandas_series_to_numpy__categorical_float64_to_float64_with_nans(self):
+        expected_float_array = np.array([1.1, 2.2, np.NaN], dtype=np.float64)
+        float_series = Series(data=[1.1, 2.2, np.NaN], dtype="category")
+
+        actual_float_array = convert_pandas_series_to_numpy(float_series, np.float64)
+
+        np.testing.assert_equal(expected_float_array, actual_float_array)
+
+    def test__convert_pandas_series_to_numpy__float64_to_float64(self):
+        expected_float_array = np.array([1.1, 2.2], dtype=np.float64)
+        float_series = Series(data=[1.1, 2.2], dtype=np.dtype(np.float64))
+
+        actual_float_array = convert_pandas_series_to_numpy(float_series, np.float64)
+
+        np.testing.assert_equal(expected_float_array, actual_float_array)
+
+    def test__convert_pandas_series_to_numpy__int64_to_int32_with_nans_throws_error(self):
+        int_series = Series(data=[1, 2, np.NaN], dtype="category")
+
+        with self.assertLogs(level="ERROR") as logger:
+            convert_pandas_series_to_numpy(int_series, np.int32)
+
+            self.assertIn("Cannot convert a pandas Series object to an integer dtype if it contains NaNs",
+                          logger.output[0])