adds table transforms

[manikyabard]

Adds TabularItem for holding table row values and some transformations for it.

[ToucheSir]

Is there a better sentinel value we can use than the literal string "missing"? Maybe missing, nothing, or a symbol :missing?

[darsnack]

Yeah we discussed during the call how we can use missing here. I'll add a review that includes that discussion.

[ToucheSir]

Instead of iterating the columns twice and having setfield repeatedly construct a namedtuple, perhaps look into a helper function that does the normalization given the tfm, column name and value? The function could check if the column is in normcols and transform it with normstats if it is.

[darsnack]

Yeah do we even need Setfield anymore if we're standardizing on a NamedTuple? We could just build the transformed data as a vector or something then construct a NamedTuple at the end.

[manikyabard]

Yeah we should be able to build it at the end.

[ToucheSir]

Same comment here about double iteration of the columns. I suppose it applies to FillMissing as well :)

[manikyabard]

Did you mean something like this?

function tfmrowvals(tfm::NormalizeRow, col, val)
    if col in tfm.cols
        colmean, colstd = tfm.dict[col]
        val = (val - colmean)/colstd
    end
    (col, val)
end

function apply(tfm::NormalizeRow, item; randstate=nothing)
    TabularItem((; 
            tfmrowvals.(
                [tfm for _ in 1:length(item.columns)],
                item.columns,
                [val for val in item.data])...
            ), 
        item.columns
    )
end

[manikyabard]

If this is better than the current implementation, we can even have a single apply which works on Union of all the transforms, and different methods for tfmrowvals.

[darsnack]

Probably still need to dispatch on each type separately in order to know which tfmrowvals function to call, right?

[manikyabard]

Yeah there will probably be 3 tfmrowvals methods.

[ToucheSir]

function apply(tfm::NormalizeRow, item; randstate=nothing)
    x = NamedTuple(Iterators.map(item.cols, item.data) do col, val
        if col in tfm.cols
            colmean, colstd = tfm.dict[col]
            val = (val - colmean)/colstd
        end
        (col, val)
    end)
end

[darsnack]

Also, I think you are using TAB for indents. Could you convert to using 4 spaces?

[darsnack]

Suggested change

	function DataAugmentation.apply(tfm::FillMissing, item::TabularItem; randstate=nothing)
	x = (; zip(item.columns, [data for data in item.data])...)
	for col in tfm.contcols
	if ismissing(x[col])
	Setfield.@set! x[col] = tfm.fmvals[col]
	end
	end
	for col in tfm.catcols
	if ismissing(x[col])
	Setfield.@set! x[col] = "missing"
	end
	end
	TabularItem(x, item.columns)
	end
	function DataAugmentation.apply(tfm::FillMissing, item::TabularItem; randstate=nothing)
	x = (; zip(item.columns, [data for data in item.data])...)
	for col in tfm.contcols
	if ismissing(x[col])
	Setfield.@set! x[col] = tfm.fmvals[col]
	end
	end
	TabularItem(x, item.columns)
	end

[darsnack]

Unless we want to allow the catergorical missings to be filled too

[darsnack]

Suggested change

	if ismissing(x[col])
	Setfield.@set! x[col] = "missing"
	end
	Setfield.@set! x[col] = tfm.pooldict[col].invindex[x[col]]
	if ismissing(x[col])
	Setfield.@set! x[col] = 0
	else
	Setfield.@set! x[col] = findfirst(tfm.categories .== col)
	end

[manikyabard]

Wouldn't this give the same value for a column?

[manikyabard]

Also seeing that the Embedding layers won't work with 0 indexing, we should probably try to avoid it.

[darsnack]

Made a typo, it should be x[col].

Then let's make missing == 1 and do + 1 for the other columns

[darsnack]

Suggested change

	function getcategorypools(catdict, catcols)
	pooldict = Dict()
	for col in catcols
	catarray = CategoricalArrays.categorical(catdict[col])
	CategoricalArrays.levels!(catarray, ["missing", CategoricalArrays.levels(catarray)...])
	pooldict[col] = catarray.pool
	end
	pooldict
	end

[darsnack]

Suggested change

	struct Categorify <: DataAugmentation.Transform
	pooldict
	categorycols
	end
	struct Categorify{T, S} <: DataAugmentation.Transform
	categories::T
	categorycols::S
	end

Two changes: swap to categories to just be a vector of the categories. I don't think we need the complexity of categorical arrays when the mapping is just the index in a list of categories passed by the user.

[manikyabard]

To reduce the complexity, we could just use the catdict used in getcategorypool directly for Categorify. A vector of vectors (or a NamedTuple) with the classes for each categorical column could work as well.

[manikyabard]

I'm not sure this will work if categories is just a vector of categorical column names as we'll have to replace the class for a categorical column with an integer, and for doing this we'll need information about all the classes which are present in a particular column.

[darsnack]

Yeah, sorry it should be a NamedTuple/Dict.

[darsnack]

Suggested change

	struct NormalizeRow <: DataAugmentation.Transform
	normstats
	normcols
	end
	struct NormalizeRow{T, S} <: DataAugmentation.Transform
	normstats::T
	normcols::S
	end

[darsnack]

Suggested change

	struct FillMissing <: DataAugmentation.Transform
	fmvals
	contcols
	catcols
	end
	struct FillMissing{T, S} <: DataAugmentation.Transform
	fmvals::T
	contcols::S
	end

[darsnack]

Yeah do we even need Setfield anymore if we're standardizing on a NamedTuple? We could just build the transformed data as a vector or something then construct a NamedTuple at the end.

[darsnack]

I also wonder if we should have a transform that maps to Flux.OneHotArray instead of just categorical indices.

[darsnack]

Suggested change

	x[idx] = ismissing(x[idx]) ? 1 : findfirst(skipmissing(x[idx] .== tfm.catdict[col])) + 1
	x[idx] = ismissing(x[idx]) ? 1 : findfirst(x[idx] .== tfm.catdict[col]) + 1

No need for skipmissing here, right? x[idx] is a value and tfm.catdict[col] is a vector of categorical values (which doesn't contain missing). findfirst is just assigning an index based on which symbol in tfm.catdict[col] matches x[idx].

[manikyabard]

Initially I was thinking if someone creates catdict using unique or something, and if somehow missing is a part of this vector then an error could be thrown, but yeah it might just be better to remove it.

[darsnack]

Probably better to map(v -> filter!(!ismissing, v), values(catdict)) when constructing the transform. We could throw a warning when that happens too.

[darsnack]

Does collect(item.data) not work?

[manikyabard]

We should be able to use that.

[darsnack]

This is looking really clean now; nice job!

[darsnack]

I think here you want to do SortedSet from DataStructures.jl. And you don't need skipmissing first, cause pushing missing onto a set that already contains it is a no-op (AbstractSets can't contain duplicates). Since it is sorted, missing will always map to the same index too (addressing @ToucheSir's concern from the call).

[darsnack]

Suggested change

	end)
	end)
	return TabularItem(x, item.columns)

[darsnack]

And all the other transforms too

[darsnack]

This can just be findfirst if we use SortedSet.

[manikyabard]

Would findfirst work when the input function involves comparing with missing?

[darsnack]

Yes, because tfm.dict[col] always contains missing, and missing is treated as any other element in the set.

[darsnack]

Ah I see what you mean. An equality comparison with missing is missing.

[darsnack]

I think as a result of this, the whole storing missing in the tfm.dict[col] is not going to work. We'll have to revert to the old filtering way + the conditional shown here.

[ToucheSir]

In that case, there's no need to store missing in the dict values either then right? The conditional is required either way.

[darsnack]

Yeah that's what I meant. Filter the missing out of the dict, and don't add it if it isn't there.

[manikyabard]

Alright, I have updated the constructor to use skipmissing and collect for the values containing missing.

[darsnack]

Is randstate an artifact from Python? Or is it part of the DataAugmentation interface? What is its role here?

[ToucheSir]

Yeah, I can't see which of these transforms requires an RNG.

[manikyabard]

Yeah, even though randstate isn't required for the tabular transformations, I put it there because it was a part of the transformation interface. I think internally for compositions, apply is called along with randstate args so everything might not work without it.

[lorenzoh]

Yeah, it's because of how the dispatch is set up.

[darsnack]

Just fix the tab issue

[lorenzoh]

Still needs some tests

[manikyabard]

The tests should be fixed now.

[ToucheSir]

Just some nits on the tests. Is there a way to trigger a CI run on the latest commit?

[ToucheSir]

Note to delete dangling comment before submission

[darsnack]

@lorenzoh will have to trigger it for "first-time contributors."