[FEA] Add options to reserve categorical indices in the Categorify() op

[gabrielspmoreira]

Is your feature request related to a problem? Please describe.
Some pipelines trust that categorical features will have some reserved values after preprocessing.
For example, for sequential features (e.g. session-based recommendation, time series, text data) it is important to reserve a value for padding (e.g. 0) in the model side, as not all sequences might have the same length.
It is also important to reserve a value (e.g. 1) to map Out-Of-Vocabulary (OOV) categorical values that might appear in the transform() (e.g. during evaluation or inference)

Describe the solution you'd like
Create one argument for Categorify() op to set the desired oov_index (for which all OOV values will be mapped during the preproc) and another argument start_index to set the first value that should be used to encode known values (e.g. 2). We can also have a similar option for null values (null_index), so that they are mapped to a known index.
In this case, if we call Categorify(oov_index=1, null_index=2, start_index=3), the index 0 could be safely used for padding for example.
I think we don't need an padding_index, because it is specific to sequential features. It is better to give users flexibility to reserve a number of categorical values by using start_index

[rnyak]

Related to NVIDIA-Merlin/Transformers4Rec#160

[EvenOldridge]

@gabrielspmoreira Do we really need that level of complexity? Can't we just specify the offset? Or does categorify work differently if you specify oov, null, and start_index ranges.

Also do we want to handle the case where people specify ridiculous values? What if I say oov = 100000000?

I think a simple reserved offset is more straightforward and less error prone.

[gabrielspmoreira]

@EvenOldridge I agree we can simplify the solution.
With the offset (start_index) we solve the issue of reserving 0 for padding.
And we could reserve the first two indices after start_index / offset for null and OOV during transform() (inference).
What do you think @benfred ?

[benfred]

I think we should simplify, and just specify a start_index with a default value of 0.

The implementation of Categorify has gotten pretty complex - so I thought it might help to put some pointers on where to get started with this:

• There are two phases in Categorify: the 'fit' which calculates the mapping of input label to categorical id, and the 'transform' which applies the mapping. We'll need to change both of these to handle this option
• For the 'fit' stage, we will need to store this option in the 'FitOptions' struct and then use in the _write_uniques function https://github.com/NVIDIA/NVTabular/blob/57855b872444fee1c249ad6f6f190a73aa8a81f3/nvtabular/ops/categorify.py#L889-L904 .
• The 'transform' code needs updated in the _encode function to set the unknown values to the start_index specified by the user. The simple case where we aren't doing any OOV hashing will involve updating these lines https://github.com/NVIDIA/NVTabular/blob/57855b872444fee1c249ad6f6f190a73aa8a81f3/nvtabular/ops/categorify.py#L1110-L1116 , but this will get more complicated with the hash bucket code. It might be worth just trying to get this working without hashing first, and then tackle hashing next.
• There is currently a 'na_sentinel' parameter on the categorify op https://github.com/NVIDIA/NVTabular/blob/57855b872444fee1c249ad6f6f190a73aa8a81f3/nvtabular/ops/categorify.py#L146-L147 . Unfortunately I don't think this works appropriately right now, and probably should be removed as an option from Categorify.

[lesnikow]

I assigned this issue to myself, but to follow what is in CONTRIBUTING.md and to make it explicit for tracking who is working on what, I am working on this issue currently.

[lesnikow]

@benfred @rjzamora Would one of you have some time today that I could meet with you to ask about how to implement this additional argument? I read over the current implementation of the class, but I am not fully understanding what the particular methods Ben outlined are supposed to do. I tagged you two since it looks like you have spent the most time working on this module. I would appreciate it if so.

[lesnikow]

@gabrielspmoreira, someone else, would you have a test case available to test the appropriate desired functionality? I found this test from tests/unit/ops/test_ops.py that tests Categorify that looks like a helpful template.

def test_categorify_lists(tmpdir, freq_threshold, cpu, dtype, vocabs):
    df = dispatch._make_df(
        {
            "Authors": [["User_A"], ["User_A", "User_E"], ["User_B", "User_C"], ["User_C"]],
            "Engaging User": ["User_B", "User_B", "User_A", "User_D"],
            "Post": [1, 2, 3, 4],
        }
    ) 

    cat_names = ["Authors", "Engaging User"]
    label_name = ["Post"]

    cat_features = cat_names >> ops.Categorify(
        out_path=str(tmpdir), freq_threshold=freq_threshold, dtype=dtype, vocabs=vocabs
    )

    workflow = nvt.Workflow(cat_features + label_name)
    df_out = workflow.fit_transform(nvt.Dataset(df, cpu=cpu)).to_ddf().compute()

    # Columns are encoded independently
    if cpu:
        assert df_out["Authors"][0].dtype == np.dtype(dtype) if dtype else np.dtype("int64")
        compare = [list(row) for row in df_out["Authors"].tolist()]
    else:
        assert df_out["Authors"].dtype == cudf.core.dtypes.ListDtype(dtype if dtype else "int64")
        compare = df_out["Authors"].to_arrow().to_pylist()

    if freq_threshold < 2 or vocabs is not None:
        assert compare == [[1], [1, 4], [3, 2], [2]]
    else:
        assert compare == [[1], [1, 0], [0, 2], [2]]

I would like to implement and test a feature add for this issue, but I am not exactly sure how to make a test to test the desired addition of Categorify with a new start_index argument.

[lesnikow]

@gabrielspmoreira I did not see how to add you as a reviewer for my draft PR, but when you have a chance, would you be able to see whether the implemented tests test your intended functionality? This draft PR consists of the commit 642e5f5.