Clean up the way shapes are computed and specified #1760
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -251,6 +251,7 @@ def __init__( | |
| sparse_names, | ||
| sparse_max, | ||
| sparse_as_dense, | ||
| batch_size, | ||
| ) | ||
|
|
||
| super().__init__( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| from dask.dataframe.utils import meta_nonempty | ||
|
|
||
| from merlin.core.dispatch import DataFrameType, annotate | ||
| from merlin.dtypes.shape import DefaultShapes | ||
|
There was a problem hiding this comment. Is this going to be a new feature of dtypes in core? There was a problem hiding this comment. Shapes are implemented as a subfield of dtypes in Core, but this isn't really intended to be a feature of dtypes in particular. We might want to hide that implementation detail a bit more thoroughly by adjusting the imports. As far as the defaults go, we just thought it was easier to read and understand than needing to remember which shapes mean what. There was a problem hiding this comment. The There was a problem hiding this comment. Expected since there is (or was) an outstanding Core PR that adds it There was a problem hiding this comment. Looks like it's still open: NVIDIA-Merlin/core#215 |
||
| from merlin.schema import Schema | ||
| from nvtabular.ops.operator import ColumnSelector, Operator | ||
|
|
||
|
|
@@ -186,10 +187,7 @@ def dependencies(self): | |
| def _compute_dtype(self, col_schema, input_schema): | ||
| col_schema = super()._compute_dtype(col_schema, input_schema) | ||
|
|
||
| agg_dtypes = { | ||
| "count": numpy.int32, | ||
| "nunique": numpy.int32, | ||
| "mean": numpy.float32, | ||
|
|
@@ -199,18 +197,26 @@ def _compute_dtype(self, col_schema, input_schema): | |
| "sum": numpy.float32, | ||
| } | ||
|
|
||
| agg = self._find_agg(col_schema, input_schema) | ||
| dtype = agg_dtypes.get(agg, col_schema.dtype) | ||
|
|
||
| return col_schema.with_dtype(dtype) | ||
|
|
||
| def _compute_shape(self, col_schema, input_schema): | ||
| agg_is_lists = {"list": True} | ||
|
|
||
| agg = self._find_agg(col_schema, input_schema) | ||
| is_list = agg_is_lists.get(agg, col_schema.is_list) | ||
|
There was a problem hiding this comment. In the case where we have fallback to the second argument of the There was a problem hiding this comment. I'm not actually sure. Are there There was a problem hiding this comment. From looking at the list of aggregations, I think everything changes the shape, either from list to scalar or from scalar to list. There was a problem hiding this comment. I wonder if the default there should actually be There was a problem hiding this comment. In practice it seems that it may not matter whether it's from merlin.io import Dataset
import nvtabular as nvt
import cudf
df = cudf.DataFrame({"a": [1, 1, 2], "b": [[10], [20], [20]]})
workflow = nvt.Workflow(["a", "b"] >> nvt.ops.Groupby(groupby_cols=["a"], aggs=["sum"]))
workflow.fit_transform(Dataset(df)).compute()
# => Raises DataError: All requested aggregations are unsupported.Some of these aggs, like Pandas for example, handles sum across lists as concatenation. import pandas as pd
df = pd.DataFrame({"a": [1, 1, 2], "b": [[10], [20], [20]]})
df.groupby("a").sum()
# =>
b
a
1 [10, 20]
2 [20]or if numpy arrays, then as an element-wise sum df = pd.DataFrame({"a": [1, 1, 2], "b": [np.array([10]), np.array([20]), np.array([20])]})
df.groupby("a").sum()
# =>
b
a
1 [30]
2 [20]
since import cudf
df = cudf.DataFrame({"a": [1, 1, 2], "b": [[10], [20], [20]]})
df.groupby("a").sum()
# => Raises DataError: All requested aggregations are unsupported.
df["b"].sum()
# => Raises TypeError: cannot perform sum with type listThere was a problem hiding this comment. So I think If we need groupby aggregations for list columns as a future feature of NVTabular this will need to be revisited. I suppose even if cudf and pandas don't natively support this we could implement this ourselves by extracting the cupy/numpy arrays from the series in our own agg function to handle list column aggregations. There was a problem hiding this comment. currently it seems that the only agg that is supported for list columns is from merlin.io import Dataset
import nvtabular as nvt
import cudf
df = cudf.DataFrame({"a": [1, 1, 2], "b": [[10], [20], [20]]})
workflow = nvt.Workflow(["a", "b"] >> nvt.ops.Groupby(groupby_cols=["a"], aggs=["list"]))
workflow.fit_transform(Dataset(df)).compute()
# =>
a b_list
0 1 [[10], [20]]
1 2 [[20]]There was a problem hiding this comment. That's great to know; I really appreciate your thoroughness in testing this out. This probably warrants a further update to the shapes here, I'll open a separate issue. There was a problem hiding this comment. Tracked here: #1763 |
||
|
|
||
| shape = DefaultShapes.LIST if is_list else DefaultShapes.SCALAR | ||
| return col_schema.with_shape(shape) | ||
|
|
||
| def _find_agg(self, col_schema, input_schema): | ||
| input_selector = ColumnSelector(input_schema.column_names) | ||
| column_mapping = self.column_mapping(input_selector) | ||
| input_column_name = column_mapping[col_schema.name][0] | ||
| agg = col_schema.name.replace(input_column_name, "").lstrip(self.name_sep) | ||
| return agg | ||
|
|
||
|
|
||
| def _aggs_for_column(col_name, agg_dict): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renaming these arguments could be out-of-scope for this PR? Since it may be a breaking change for something that uses this function. It may be clearer to separate this into a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We couldn't really figure out what this function was supposed to do without the renames, so we went for it. The function name starts with an
_so we've appropriately signaled that external code shouldn't depend on its stability. The two places in the NVTabular that use it call it via argument order instead of specifying the names, so I understand the caution but I think we're okay.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it will be a safe change, and the names make things clearer :). We may consider removing this functon along with the other loader code to follow-up on the promise here in one of the upcoming releases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And by that point we'll hopefully have updated the dataloader API to a point where this augment_schema function is no longer required either here or the copies in Transformers4Rec and in Merlin Models. The existence of this function suggests to me that there's something missing from the dataloader API as it currently exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the something that's missing is "transforms implemented as operators that provide schema tracking" 😺