Added ShrunkCovariance #309
Conversation
Co-authored-by: Mateusz Sluszniak <[email protected]>
Co-authored-by: José Valim <[email protected]>
| {x - location, location} | ||
| end | ||
|
|
||
| defn empirical_covariance(x) do |
There was a problem hiding this comment.
You should be able to use Nx.covariance/2 instead.
There was a problem hiding this comment.
I don't think so, look at the PR description
There was a problem hiding this comment.
Right, I forgot to ask: @norm4nn did you try setting ddof: 0 when calling Nx.covariance/2?
There was a problem hiding this comment.
Sorry, ddof: 0 is default. It's strange that the tests are failing, because Nx.covariance/2 does exactly what is implemented here. Could it be the case that the data in your test is not centered and you are using Nx.covariance/2?
There was a problem hiding this comment.
Yeah, you are right! This is the case, I will fix this on Thursday.
Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Krsto Proroković <[email protected]>
| @@ -182,9 +149,9 @@ defmodule Scholar.Covariance.LedoitWolf do | |||
|
|
|||
| defnp ledoit_wolf_shrinkage_complex(x) do | |||
| {num_samples, num_features} = Nx.shape(x) | |||
| emp_cov = empirical_covariance(x) | |||
| emp_cov = Nx.covariance(x) | |||
There was a problem hiding this comment.
Is x centered here? If yes, maybe reverting it to empirical_covariance might be better. 😅
There was a problem hiding this comment.
Hmm, I don't think empirical_covariance/1 was centering x, if that's what you're asking about. However, I'm still a bit confused as to why, when I used empirical_covariance/1 instead of Nx.covariance/2, the Covariance.*.fit/2 functions produced the same output as the scikit-learn versions when I passed a non-centered x and set assume_centered? to true. While I feel this case is invalid and can't think of any practical use case for passing such arguments, it still feels odd that the output in this edge case differs when using Nx.covariance/2 compared to the scikit-learn version. It seems empirical_covariance might not operate exactly the same way as Nx.covariance/2 , so I also like the idea of reverting it to empirical_covariance.
There was a problem hiding this comment.
I think you need
| emp_cov = Nx.covariance(x) | |
| emp_cov = Nx.covariance(x, Nx.reshape(0, {1})) |
for the code to be totally equivalent.
empirical_covariance was blindly assuming the mean is 0
There was a problem hiding this comment.
empirical_covariance was blindly assuming the mean is 0
Yes, because the data should be centered before. That is why using empirical_covariance might be better idea. Like this the data won't be centered twice.
There was a problem hiding this comment.
So you want me to change it back to empirical_covariance version?
There was a problem hiding this comment.
Sorry for the delayed response! I am still checking why empirical_covariance and Nx.covariance are giving completely different results.
There was a problem hiding this comment.
@norm4nn let's revert to empirical_covariance for now so we can merge this and get unstuck. :)
norm4nn
force-pushed
the
shrunk_covariance
branch
from
4297069
to
f72d08f
Compare
|
💚 💙 💜 💛 ❤️ |
What has been added:
Following up on #304: I added the
?sign to theassume_centeredoption, but I'm unsure about replacingempirical_covariance/1withNx.covariance/3because tests don’t pass when I make that change. I believe the tests should stay as they are, since I’ve already verified the results against scikit-learn.