stumpy icon indicating copy to clipboard operation
stumpy copied to clipboard

passing-input-with-finite-value to `core._mass` is being violated

Open NimaSarajpoor opened this issue 1 year ago • 6 comments

The function core.mass(Q, T, ...) calls core.preprocess(T, m, ...), which return the following values:

  • T
  • M_T
  • Σ_T
  • T_subseq_isconstant

We then pass these values to core._mass. However, these M_T and Σ_T can contain non-finite value if the original T has non-finite value. In fact, if you just follow the function core._mass and go the callee function and then continue till you reach the very end of the road, you can see that at the end, we use that in the function core._calculate_squared_distance to determine whether a distance should be infinite or not.

https://github.com/TDAmeritrade/stumpy/blob/3077d0ddfb315464321dc86f8ec3bf2cab9ce3b1/stumpy/core.py#L1093-L1094

In the function core._calculate_squared_distance, the flag is correctly set. Was wondering if should do the same for core._mass?

NimaSarajpoor avatar Jul 21 '24 01:07 NimaSarajpoor

What exactly are you proposing? To set fastmath={"nsz", "arcp", "contract", "afn", "reassoc"} for core._mass? Maybe... I don't know.

seanlaw avatar Jul 21 '24 02:07 seanlaw

Right!

I am now thinking what if we just avoid setting fastmath here for core._mass because this function does not do any arithmetic operations. I can try it out and check its impact on performance if there is any.

NimaSarajpoor avatar Jul 21 '24 02:07 NimaSarajpoor

In the following, A --> B means function A calls function B.

# in stumpy.core
mass --> _mass
_mass --> calculate_distance_profile
calculate_distance_profile -->  _calculate_squared_distance_profile
_calculate_squared_distance_profile --> _calculate_squared_distance

I removed the fastmath flag for all expect the last one, which comes with fastmath={"nsz", "arcp", "contract", "afn", "reassoc"}.

I then compared the performance of core.mass for the following input:

seed = 0
np.random.seed(seed)
Q = np.random.rand(100)
T = np.random.rand(1_000_000)

I ran the following script:

import numpy as np
import time
import stumpy

def check_mass_performance():
    n_iter = 100

    seed = 0
    np.random.seed(seed)
    Q = np.random.rand(100)
    T = np.random.rand(1000000)

    t_lst = []
    for _ in range(n_iter):
        start = time.time()
        stumpy.mass(Q, T)
        t_lst.append(time.time() - start)
    
    return np.mean(t_lst[1:]), np.std(t_lst[1:])


if __name__ == "__main__":
    out = check_mass_performance()
    print(f'mean: {out[0]}, std: {out[1]}' )

And I ran it for several times.

Running time mean std
case 1 0.096-0.097 ~0.002
case 2 0.096-0.097 ~0.002

where case 1 is the existing version and case 2 is when the fastmath flags are removed except for core. _calculate_squared_distance. Although I did not do any statistical test, it seems that there is no certain gain in having fastmath flags on all those njit functions.

NimaSarajpoor avatar Jul 28 '24 12:07 NimaSarajpoor

I am now thinking what if we just avoid setting fastmath here for core._mass because this function does not do any arithmetic operations. I can try it out and check its impact on performance if there is any.

I see. When there is no speed gain, I would rather be explicit rather than implicit. In this case, it appears that the inputs/outputs can all contain non-finite values. This means that fastmath=True is "wrong". I would simply make every level of the nested function calls fastmath={"nsz", "arcp", "contract", "afn", "reassoc"}. That would be my preference as this would be very explicit and one would never have to question whether each function was allowed to have non-finite values (the answer is "yes!").

I think this should be handled together with #708 though

seanlaw avatar Aug 03 '24 18:08 seanlaw

This means that fastmath=True is "wrong". I would simply make every level of the nested function calls fastmath={"nsz", "arcp", "contract", "afn", "reassoc"}. That would be my preference as this would be very explicit

That's a good idea! Got it!!

I think this should be handled together with https://github.com/TDAmeritrade/stumpy/issues/708 though

Okay. So, the plan is to first solve #708 (and this issue, i.e. #1011), and then resume the work on #1012 as suggested in https://github.com/TDAmeritrade/stumpy/pull/1012#discussion_r1702858438.

NimaSarajpoor avatar Aug 04 '24 06:08 NimaSarajpoor

Yes, thank you @NimaSarajpoor!

seanlaw avatar Aug 04 '24 11:08 seanlaw