Conversation
…ture_types fixed to support sparse.COO, tests
eroell
left a comment
eroell
left a comment
There was a problem hiding this comment.
Please add a benchmark with compute time and memory consumption in the PR comment, where a large dense 3D and a large sparse 3D, and also the 2D sparse and dense arrays are tested.
The sparse COO version should run for a ~500k x 1000k x 90 tensor of about 1% data density.
| ) -> list[tuple[Literal["categorical", "numeric"], bool]]: | ||
| """Detect the feature type of every variable (axis 1) of a numeric `sparse.COO` array, without densifying. | ||
|
|
||
| A `sparse.COO` array is always numeric, so classification only needs to decide, per variable, whether its values |
|
|
||
| if issparse(X): | ||
| X = to_dense(X) | ||
| if isinstance(X, sparse.COO): |
There was a problem hiding this comment.
Could we, now that we're at it, have this function single-dispatched for the different array types we're using? sparse.COO, scipy sparse CSR, scipy Sparse CSC. Without densifying
|
|
||
| @_harmonize_missing_values_numeric.register(sparse.COO) | ||
| def _(X: sparse.COO, *, var_names: pd.Index, vars: Iterable[str] | None) -> tuple[sparse.COO, bool]: | ||
| """Swap a numeric :class:`sparse.COO` array's zero fill value to `np.nan`, keeping `vars` columns real. |
There was a problem hiding this comment.
Less comments in internal functions please, they get very easily out of sync with actual code if they're that long
| assert missing_value_string not in edata.layers[DEFAULT_TEM_LAYER_NAME].flatten() | ||
|
|
||
|
|
||
| def test_harmonize_missing_values_sparse_coo(): |
There was a problem hiding this comment.
Can we also have parametrized tests that show that the feature type inferences behaves the same across all array types?
Whenever we update a function to work with other array types, we most likely want to update existing tests with parametrization of the new array type :)
| fill_is_nan = np.issubdtype(X.dtype, np.floating) and np.isnan(X.fill_value) | ||
|
|
||
| results = [] | ||
| for j in range(n_vars): |
There was a problem hiding this comment.
Is claude really sure that this for loop is needed?
2 participants
Both functions previously either crashed or silently no-op'd on a sparse.COO X/layer. This adds proper support to both, without ever densifying the array.
infer_feature_types_detect_feature_types_sparse_coohelper implements this; infer_feature_types branches to it for sparse.COO input and keeps the existing dataframe-based path unchanged for everything else.harmonize_missing_valuesfill_valuefrom 0 to NaN, so the array stays sparse)varsargument: variable names whose 0 is a real measured value, not missing. Previously-implicit zeros of these columns are materialized as explicit stored 0 entries (so they dont get changed to the new NaN fill value); every other column stays sparseTests: added coverage in
test_feature_types.pyfor 2D/3D sparse.COO harmonization, vars exclusion, an unknown-var error, scipy.sparse being unaffected, and sparse.COO feature-type inference (including binary detection and the all-NaN-column error). Two pre-existing IO round-trip tests (test_h5ed.py/test_zarr.py) updated to passharmonize_missing_values=False, since they test binsparse encoding fidelity specifically and predate this behavior change.