Zero and inf fix

[sergeyf]

Re: #3

@iskandr Please check it out to make sure these changes don't fundamentally break something else.

[amueller]

Maybe add a minimal regression test using

X = np.array([[1, 1, np.NaN], [1, 1, 1]])
knnimpute.knn_impute_few_observed(X, np.isnan(X), k=1)

?

[sergeyf]

Yup, good idea. It's in there.

[iskandr]

In [1]: np.array([1,2,np.inf]).max()
Out[1]: inf

[sergeyf]

Oh right, I should do D[np.isfinite(D)].max()

[sergeyf]

Err unless that makes D empty. That's impossible, right?

[iskandr]

Well, I guess you could play it very safe and have:

finite_distances = D[np.isfinite(D)]
if len(finite_distances) > 0:
   max_dist = 1e6 * max(1, finite_distances.max())
else:
   max_dist = 1e6

[iskandr]

If infinity is in the distance matrix, won't this make max_dist infinity?

In [1]: np.array([1,2,np.inf]).max()
Out[1]: inf

[iskandr]

What does max_dist_multiplier do?

[sergeyf]

Oh right, it's supposed to be in the code instead of 1e6.

[iskandr]

FYI, this is going to break the few_observed_entries implementation, which depends on being able to tell which distances are infinite due to no overlap between samples.

    effective_infinity = 10 ** 6 * D[finite_distance_distance_mask].max()
    D[~finite_distance_distance_mask] = effective_infinity
    D_sorted = np.argsort(D, axis=1)
    inv_D = 1.0 / D
    D_valid_mask = D < effective_infinity

[iskandr]

One way to fix this for few_observed_entries would be to have knn_initialize return max_dist. I can do that as a second PR if you'd like.

[sergeyf]

@iskandr I think I fixed that a few commits back?

    effective_infinity = D[0, 0] # since diagonal was replaced by max_dist
    D_valid_mask = D < effective_infinity
    valid_distances_per_row = D_valid_mask.sum(axis=1)

[iskandr]

Didn't see that, looks good.

…

On Wed, Mar 15, 2017 at 5:12 PM, Sergey Feldman ***@***.***> wrote: @iskandr <https://github.com/iskandr> I think I fixed that a few commits back? effective_infinity = D[0, 0] # since diagonal was replaced by max_dist D_valid_mask = D < effective_infinity valid_distances_per_row = D_valid_mask.sum(axis=1) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC9OQ1go7tRFllxb9ImY030nog17Srjks5rmFQugaJpZM4Med2Z> .

[iskandr]

Test failure: NameError: global name 'knnimpute' is not defined

[sergeyf]

That's weird. I fixed that:

+def test_knn_minimal():
 +    X = np.array([[1, 1, np.NaN], [1, 1, 1]])
 +    res = knn_impute_few_observed(X, np.isnan(X), k=1)
 +    assert np.isnan(res).any() == False, \
 +        "Basic example did not get imputed: %s" % res 

[sergeyf]

Did you get the latest version?

[iskandr]

I took a peek at Travis but I guess it's behind on builds. Wonder why it's so slow today.

[coveralls]

Coverage decreased (-0.4%) to 95.575% when pulling 324309e on zero_and_inf_fix into d045b5c on master.

[coveralls]

Coverage decreased (-0.4%) to 95.575% when pulling 324309e on zero_and_inf_fix into d045b5c on master.

[coveralls]

Coverage decreased (-0.4%) to 95.575% when pulling 324309e on zero_and_inf_fix into d045b5c on master.

[iskandr]

Not sure which library this bubbles up from, but we get a warning on Python 2.7:

Problem importing module variables.py: No module named backports.functools_lru_cache

Possibly fixed by depending on https://pypi.python.org/pypi/backports.functools_lru_cache/1.3? I can look into it later.

[coveralls]

Coverage decreased (-0.4%) to 95.575% when pulling 324309e on zero_and_inf_fix into d045b5c on master.

[coveralls]

Coverage decreased (-0.4%) to 95.575% when pulling 324309e on zero_and_inf_fix into d045b5c on master.