Add KNNImputer

[srzeszut]

I have added the KNNImputer and I am currently implementing tests to ensure that it behaves as expected across various scenarios, including edge cases.

[josevalim]

This check does not really work in Nx. If you call fit inside Nx.Defn.jit, then x is an expression, and we can't read its values to find out if there is a nan or not. The best we can do is to remove this check and document it.

[srzeszut]

I found this check in simple imputer
https://github.com/elixir-nx/scholar/blob/main/lib/scholar/impute/simple_imputer.ex
Are you sure it won't work?

[josevalim]

It is also broken there. :)

[josevalim]

I have fixed it there: c024c5b

[josevalim]

Same here, this code won't work because, when you have an expression, you can't get a number from it. Can we remove this check? What happens if we don't check for this condition?

[josevalim]

You can test this by calling fit after jitting it with Nx.Defn.fit.

[josevalim]

I am not sure if this check is guaranteed to work, given two NaNs are not guaranteed to be equal. Using Nx.is_nan would be more appropriate.

[msluszniak]

Use Nx.is_nan here NaN is not equal to itself

[msluszniak]

Generally, when you write in defn, you don't need to wrap this zero in Nx.tensor. I prefer to explicitly use Nx.<type> or Nx.tensor(x, type: type) to indicate the type of the tensor. Now, there are some cases where imputter has fixed type like :f32. I think that this might cause undesired upcasts when e.g. I have tensor of type :bf16. So I suggest to check if there are any unwanted casts / upcast.

[srzeszut]

I changed it but I don't know how to change this line
row_distances = Nx.iota({rows}, type: {:f, 32})
because i don't know what the type calculated distance will be at this point

[msluszniak]

As mentioned in comment up, try to replace "bare" numbers with typed tensors

[msluszniak]

I think it should be written with double t KNNImputter like formatter etc.

[msluszniak]

Thanks for the PR, I dropped some comments :))

[krstopro]

Hi @srzeszut and thanks for the pull request. I’m traveling now and don’t have my laptop with me. Will be back this Sunday, so I will have a look probably next week.

[srzeszut]

Thanks for the review, I apply suggested changes and left some comments.

[josevalim]

error messages start in lowercase. :)

Suggested change

	"Number of neighbors rows must be less than number valid of rows - 1 (valid row is row with more than 1 non nan value)"
	"number of neighbors rows must be less than number valid of rows - 1 (valid row is row with more than 1 non nan value)"

[josevalim]

Can you please add some tests? In particular, please add a test where you call jit this function and then you call it: Nx.Defn.jit(...).(arg1, arg2). It should reveal some errors around here. :)

[srzeszut]

I added tests and checked it. I removed those checks and added them in the description

[josevalim]

Suggested change

	`n_neighbors` nearest neighbors found in the training set. Two samples are
	close if the features that neither is missing are close.
	`n_neighbors` nearest neighbors found in the training set. Two samples are
	close if the features that neither is missing are close.

[josevalim]

Please try to break this long line :)

[josevalim]

Test names start in lowercase :)

Suggested change

	test "Wrong impute rank" do
	x = Nx.tensor([1, 2, 2, 3])
	assert_raise ArgumentError,
	"Wrong input rank. Expected: 2, got: 1",
	fn ->
	KNNImputter.fit(x, missing_values: 1, number_of_neighbors: 2)
	end
	end
	test "Invalid n_neighbors value" do
	test "invalid impute rank" do
	x = Nx.tensor([1, 2, 2, 3])
	assert_raise ArgumentError,
	"Wrong input rank. Expected: 2, got: 1",
	fn ->
	KNNImputter.fit(x, missing_values: 1, number_of_neighbors: 2)
	end
	end
	test "invalid n_neighbors value" do

[josevalim]

I dropped the last round of nitpicks and we are good to go!

[krstopro]

First review. Some features we might wanna have:

• Make k-NN algorithm configurable.
• Make the metric configurable.

You can leave these for another pull request. Have a look at e.g. KNNClassifier how it is done over there.
I should have another look tonight.

[krstopro]

I would suggest changing this to num_neighbors to be consistent with the rest of Scholar.

[krstopro]

Several minor comments for now. I have to go through the code at least once more as I don't exactly understand the logic here.

[krstopro]

You should be able to use == instead of Nx.equal/2.

[polvalente]

This is a deftransform, so Nx.equal is the proper function. == will be Elixir.Kernel.==

[krstopro]

I would move this line above so that you don't access opts[:missing_values] multiple times.

[krstopro]

You can use < instead of Nx.less/2 over here.

[krstopro]

Same here.

[krstopro]

I think if mask[row][col] do should work here.

[polvalente]

Suggested change

	* `number_of_neighbors` is a positive integer.
	* number of neighbors must be less than number valid of rows - 1 (valid row is row with more than 1 non nan value) otherwise it is better to use simple imputter
	* The number of neighbors must be less than the number of valid rows - 1.
	A valid row is a row with more than 1 non-NaN values. Otherwise it is better to use a simpler imputer.

[polvalente]

Suggested change

	* when you set a value different than :nan in `missing_values` there should be no NaNs in the input tensor
	* When you set a value different than `:nan` in `missing_values` there should be no NaNs in the input tensor

[polvalente]

Suggested change

	[`Nx.Constant.nan/0`](https://hexdocs.pm/nx/Nx.Constants.html#nan/0) values.
	[`Nx.Constants.nan/0`](https://hexdocs.pm/nx/Nx.Constants.html#nan/0) values.

[polvalente]

Do you need the explicit linking in hexdoc?

[polvalente]

Suggested change

	* `:missing_values` - the same value as in `:missing_values`
	* `:missing_values` - the same value as in the `:missing_values` option

[polvalente]

Suggested change

	placeholder_value = Nx.Constants.nan() |> Nx.tensor()
	placeholder_value = Nx.Constants.nan()

[polvalente]

you probably want to pass the input type here to avoid upcasts

[polvalente]

Suggested change

	type: {:or, [:float, :integer, {:in, [:nan]}]},
	type: {:or, [:float, :integer, {:in, [:nan]}]},

I believe this should allow :infinity and :neg_infinity too for completeness

[polvalente]

Suggested change

	indices =
	[Nx.stack(row), Nx.stack(col)]
	|> Nx.concatenate()
	|> Nx.stack()
	indices = Nx.stack([row, col]) |> Nx.reshape({1, 2})

If I read the code correctly, row and col are scalars and this should yield the same result

[polvalente]

Suggested change

	values_to_impute = Nx.indexed_put(values_to_impute, indices, Nx.stack(neighbor_avg))
	values_to_impute = Nx.put_slice(values_to_impute, [row, col], Nx.reshape(neighbor_avg, {1, 1}))

I think this is even simpler

[polvalente]

try this:

  potential_donors = Nx.vectorize(x, :rows)
  distances = nan_euclidean(row_with_value_to_fill, nan_col, potential_donors) |> Nx.devectorize()
  row_distances = Nx.indexed_put(distances, [i], Nx.Constants.infinity())

[srzeszut]

Thanks for all the comments, I applied your suggested changes to the code.

[josevalim]

💚 💙 💜 💛 ❤️