Handling NaN/Inf in core.mass and stamp.mass

[seanlaw]

In this comment a user is trying to use core.mass (or stamp.mass) to compute a distance profile but their data contains np.nan/np.inf. Currently, nan/inf handling is handled at a higher abstraction level (which is fine) but it would be beneficial to allow core.mass/stamp.mass to automatically handle nan/inf since these functions will inevitably be called upon when finding matches to a single known pattern/motif.

[seanlaw]

@mexxexx Taking a look at the previous comment above, can you think of a way where the user can easily use core.mass or stamp.mass directly with data that contains np.nan/np.inf without having to manually perform the pre-processing step themselves? Right now, you'd have to do something like:

T = np.random.rand(1000)
m = 50

# Some pre-processing steps that is usually handled by `stumpy.stump` but you need to do this manually
T_copy = T.copy()  # Leave original time series unchanged
T_copy[np.isinf(T_copy)] = np.nan
M_T, Σ_T = core.compute_mean_std(T_copy, m)
T_copy[np.isnan(T_copy)] = 0
zone = excl_zone = int(np.ceil(m / 4))

# You compute the matrix profile and find a motif at `i = 10`
Q = T_copy[10: 10 + m]
P, I = stamp.mass(Q, T_copy, M_T, Σ_T, trivial_idx=10, excl_zone=zone)

There are several issues that one would need to solved:

1. Making a copy of T so as not to overwrite the original time series
2. Convert all inf to nan before computing the sliding window mean and standard deviation
3. Convert nan to zero before calling core.mass or stamp.mass

Do you have any thoughts?

[mihailescum]

There is some nan inf handling in core.mass, so it might be possible to do all this work there. We would need to have a look where core.mass is called and of this would lead to any conflicts, but from the first glance it should be fine.

If the query contains nans, an infinite distance profile is returned, so there should be no problem doing the same if the query contains infs. And if there are nans in the time series, they should be handled correctly too, so just adding some more checks shouldn't be a problem.

[mihailescum]

I think in general there should be only one mass function public to the user, and I would choose core.mass, because no exclusion zone is applied there and if you only want the distance profile you probably don't need the exclusion zone anyways.

[seanlaw]

I'm trying to apply stumpy.core.mass, and I'm getting back mostly nans. Even though it seems like I'm calling correctly looking at https://github.com/TDAmeritrade/stumpy/blob/master/stumpy/core.py.

It looks to me like it can't handle a time series with nans in it. Just tested on the time series without those in between each conversation and it worked.

@codyschank Would you mind supplying us with a small/short example Q and T pair that fails with core.mass failed for you? This would help us test and improve the code. Thanks in advance!

[seanlaw]

I think in general there should be only one mass function public to the user, and I would choose core.mass, because no exclusion zone is applied there and if you only want the distance profile you probably don't need the exclusion zone anyways.

I agree with you. Do you feel like stamp.mass is doing too much?

[mihailescum]

I don't think it's doing to much, but maybe we should think about remaining it to capture that it is actually computing a single matrix profile value based on the distance profile. If I remember correctly the authors call only the calculation of the distance profile MASS.

[seanlaw]

Ahh, you're right. I totally forgot that stamp.mass only returns single a matrix profile. We should probably update the documentation to reflect this. Right now, it says that both are ndarray but the P is probably just a float while I can include the left/right indices.

[seanlaw]

maybe we should think about remaining it to capture that it is actually computing a single matrix profile value based on the distance profile

Okay, I am assuming you mean "renaming it" and I think that's a good idea. What should we rename it to?

If I remember correctly the authors call only the calculation of the distance profile MASS.

Yes, I think you're right!

[mihailescum]

Okay, I am assuming you mean "renaming it" and I think that's a good idea. What should we rename it to?

Maybe something like _calculate_matrix_profile_value? Also, the docstring has to be update to explain better what the function in doing.

[seanlaw]

Maybe something like _calculate_matrix_profile_value? Also, the docstring has to be update to explain better what the function in doing.

How would you feel about stamp.mass_PI? I think it captures the fact that it is using the mass algorithm to compute the distance profile (and not a different algo) and then return the P and I. If you are agreeable then I can go ahead and change this (and update all areas of the code that calls stamp.mass to stamp.mass_PI and update the docs too.

[mihailescum]

That works. Maybe make the function private?

[seanlaw]

Maybe make the function private?

Hmmm, it almost feels like we need two functions. One for stamp._mass_PI that returns P and I and possibly another one stamp.mass that returns the distance profile but with the exclusion zone set to inf.

It feels clunky to simply call core.mass to get the distance profile and then have to handle the exclusion zone yourself (as the end user). Considering that you and I have both had issues with choosing a consistent exclusion zone, it seems like it would be better to do it for the user so that it is always consistent beyond having the user define m.

[mihailescum]

I agree, that makes sense.

[seanlaw]

There is some nan inf handling in core.mass, so it might be possible to do all this work there. We would need to have a look where core.mass is called and of this would lead to any conflicts, but from the first glance it should be fine.

If the query contains nans, an infinite distance profile is returned, so there should be no problem doing the same if the query contains infs. And if there are nans in the time series, they should be handled correctly too, so just adding some more checks shouldn't be a problem.

@mexxexx Do you think you could provide a checklist on the steps that you think are necessary to get this to work? And then, I could try to take a stab at it and report back if I encounter any challenges