Fix #695 Add "subseq_isconstant" param to API #789

NimaSarajpoor · 2023-01-28T16:43:10Z

No description provided.

codecov-commenter · 2023-01-28T16:56:46Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (c7d5321) 99.24% compared to head (f1519ea) 99.25%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #789    +/-   ##
========================================
  Coverage   99.24%   99.25%            
========================================
  Files          82       82            
  Lines       12974    13121   +147     
========================================
+ Hits        12876    13023   +147     
  Misses         98       98

Impacted Files	Coverage Δ
stumpy/core.py	`100.00% <100.00%> (ø)`
stumpy/gpu_stump.py	`100.00% <100.00%> (ø)`
stumpy/stump.py	`100.00% <100.00%> (ø)`
stumpy/stumped.py	`100.00% <100.00%> (ø)`
tests/naive.py	`100.00% <100.00%> (ø)`
tests/test_core.py	`100.00% <100.00%> (ø)`
tests/test_gpu_stump.py	`100.00% <100.00%> (ø)`
tests/test_stump.py	`100.00% <100.00%> (ø)`
tests/test_stumped.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

stumpy/stump.py

NimaSarajpoor · 2023-01-28T18:38:49Z

seanlaw · 2023-01-29T02:07:15Z

For the sake of consistency, I think we should add the "subseq_isconstant" param to the following modules as well.

What about stumpi, mstumped, ostinato, ostinatoed, gpu_ostinato, mpdisted, gpu_mpdist, stimped, gpu_stimp?

NimaSarajpoor · 2023-01-29T20:12:54Z

What about stumpi, mstumped, ostinato, ostinatoed, gpu_ostinato, mpdisted, gpu_mpdist, stimped, gpu_stimp?

Well... I haven't explored all modules yet but we should defintely check them. I also need to check the ones I mentioned in my previous comment again to make sure the implementation is doable/ reasonable.

For instance: I have some difficulty in understanding how users can get benefit from this feature when data is updated dynamically. In stumpi, matrix profile is computed in the context of an streaming data. While users may provide their own input for subseq_isconstant for the initial input T, I do not understand how this should be updated as new data being inserted to the time series. (alternative option: ask users to provide stddev threshold)

What about PAN matrix profile? I haven't studied its module yet but it seems it computes matrix profile for different window length. So, in that case, I think we should avoid allowing user to insert their own "subseq_isconstant" array for just one window size. Or, we should allow them to provide this array for each window size.

I will try to explore modules one by one to see if we can add this new support for them. Please let me know if you have any suggestion.

seanlaw · 2023-01-30T14:50:04Z

Please let me know if you have any suggestion.

So, I'm wondering if we could do something like:

# core.py
import inspect

def rolling_isconstant(a, w, custom_func=None):
    """
    """
    axis = a.ndim - 1
    rolling_isconstant_func = _rolling_isconstant

    if custom_func is not None:
        custom_func_args = set(inspect.signature(some_func).parameters.keys()
        if len(custom_func_args.difference(set(['a', 'w']))):
            rolling_isconstant_func = custom_func
        else:
            msg = "Incompatible parameters found in custom function (in `rolling_isconstant`)"
            warnings.warn(msg)

    return np.apply_along_axis(
        lambda a_row, w: rolling_isconstant_func(a_row, w), axis=axis, arr=a, w=w
    )

And then, in a function like stumpy.stump (or other API functions), we can do something like:

if callable(T_subseq_isconstant):
    isconstant_func = T_subseq_isconstant  # save the function in case we need it for later??
    T_subseq_isconstant = core.rolling_isconstant(T, m, isconstant_func)
if T_subseq_isconstant is None:
    T_subseq_isconstant = core.rolling_isconstant(T, m)

I haven't thought this through and it is still somewhat convoluted but some variation of this might work after we clean it up. It should even be usable for stimp since the user's custom function would always be applied in place of our _rolling_isconstant and it would be dynamic for each window size (i.e., stimp would only accept a custom function for T_subseq_isconstant and not a numpy array).

Again, just a soft proposal for you to consider.

NimaSarajpoor · 2023-02-01T03:58:56Z

It should even be usable for stimp since the user's custom function would always be applied in place of our _rolling_isconstant and it would be dynamic for each window size (i.e., stimp would only accept a custom function for T_subseq_isconstant and not a numpy array).

This actually sounds great! We can also let user know that subsequences with at least one nan or inf will be treated as "not constant" regardless of the provided custom function. (Otherwise, we need to modify T_subseq_isfinite)

environment.yml

…kind

seanlaw

Added one minor suggestion. Also, I wonder if it makes sense to break this up into smaller individual PRs rather than one giant one? The current size of this PR is okay but maybe we merge this one when it is ready and then add other files in a separate PR?

stumpy/core.py

NimaSarajpoor · 2023-03-10T04:08:00Z

@seanlaw

I wonder if it makes sense to break this up into smaller individual PRs rather than one giant one? The current size of this PR is okay but maybe we merge this one when it is ready and then add other files in a separate PR?

According to our experience in top-k PR, I think what you are suggesting is reasonable. I checked out the changed files and I think this PR is ready. We already added the param to stump, stumped, and gpu_stump. So, I think it is good to be merged. Please allow me to address your comment and take a look at the changes for one last time.

NimaSarajpoor · 2023-03-10T05:10:39Z

@seanlaw

Please allow me to address your comment and take a look at the changes for one last time.

[Update]
I addressed your comment, and checked the changed files. They look good to me. Please feel free to merge.

seanlaw · 2023-03-10T14:58:09Z

@NimaSarajpoor It looks like we are missing some code coverage:

Name                 Stmts   Miss  Cover   Missing
--------------------------------------------------
tests/naive.py        1216      1    99%   243
tests/test_core.py     993      7    99%   89, 1577, 1583, 1589, 1595, 1601, 1607
--------------------------------------------------
TOTAL                13037      8    99%

Note that even though these are are naive.py and test_core.py, this implies that some paths are not traversed within these functions, which is a problem (i.e., please do not simply do pragma no cover)

NimaSarajpoor · 2023-03-10T21:24:28Z

@seanlaw

It looks like we are missing some code coverage

I really need to understand the importance of checking code coverage by heart :)

tests/naive.py

NimaSarajpoor · 2023-03-10T21:30:27Z

tests/test_core.py

+def test_find_incompatible_args():
+    # case1: having exact required argument
+    def func_case1(x, y):
+        return


This function and other functions below it are designed to test the functionality of core._find_incompatible_args. However, since we do not call these functions (right?), these functions are skipped according to the result shown in code coverage. Any suggestion @seanlaw ?

@seanlaw
FYI: To fix code coverage, I added # pragma: no cover here and for the next few functions.

seanlaw · 2023-03-10T21:47:35Z

I really need to understand the importance of checking code coverage by heart :)

First, I recommend running the tests locally first for non-trivial PRs. Having said that, I'm going to add something to our coverage reporting to force it to fail if the coverage is below 100%. Hopefully, that'll help. I should've done it a long time ago

NimaSarajpoor · 2023-03-10T22:04:11Z

First, I recommend running the tests locally first for non-trivial PRs.

Right! Need to keep that in mind!

I'm going to add something to our coverage reporting to force it to fail if the coverage is below 100%.

Cool!! I think that would be a great idea!

seanlaw · 2023-03-10T22:28:25Z

@NimaSarajpoor I just pushed a new commit that I think/hope will cause a failure. Would you mind pulling it into this branch?

NimaSarajpoor · 2023-03-10T23:02:58Z

That was quick :) I will update my branch.

seanlaw · 2023-03-11T01:37:19Z

Please pull the latest commit (the last one wasn't enough).

NimaSarajpoor · 2023-03-11T04:19:05Z

@seanlaw
I ran the test on google colab, and I got this:

Name                 Stmts   Miss  Cover   Missing
--------------------------------------------------
tests/naive.py        1216      1    99%   243
tests/test_core.py     993      7    99%   89, 1577, 1583, 1589, 1595, 1601, 1607
--------------------------------------------------
TOTAL                13037      8    99%

78 files skipped due to complete coverage.
Cleaning Up

I am going to push the commits...

NimaSarajpoor · 2023-03-11T05:00:05Z

We got error. That is good. I will wait till you handle the error occured in your last commit (You may want to see this: nedbat/coveragepy#198)

seanlaw · 2023-03-11T21:33:01Z

We got error. That is good. I will wait till you handle the error occured in your last commit (You may want to see this: nedbat/coveragepy#198)

It should be fixed now.

NimaSarajpoor · 2023-03-12T04:21:56Z

@seanlaw
Please let me know if I should take care of anything else for this PR.

seanlaw · 2023-03-12T11:06:51Z

@NimaSarajpoor Everything looks good here. Merging now. Thanks!

NimaSarajpoor added 5 commits January 28, 2023 02:27

add T_subseq_isconstant param to naive stump

5ab291c

add test for new param in naive, expected error

3c93079

add param to a core function to increase flexibility for user

8da13b9

add new param to public API to increase flexibility for user

9d4f780

fix decorator

555ae13

seanlaw reviewed Jan 28, 2023

View reviewed changes

stumpy/stump.py Outdated Show resolved Hide resolved

NimaSarajpoor added 2 commits February 2, 2023 01:14

add custom_func for rolling_isconstant

3b2097f

fix if block

5be22a7

NimaSarajpoor force-pushed the subseq_constant_in_API branch from 5f6704d to 5be22a7 Compare February 4, 2023 05:53

NimaSarajpoor added 3 commits February 4, 2023 00:56

change black minimum version to resolve trailing-comma issue

3bce443

fix format with latest version of black

9fb636b

retreive setting for black minimum version

00a7d15

seanlaw reviewed Feb 6, 2023

View reviewed changes

environment.yml Outdated Show resolved Hide resolved

NimaSarajpoor added 11 commits February 6, 2023 07:22

replace array with a custom function

e7160af

replace array with func as new param for determining constant subseqs

ed0c2d9

revise core functions to have param isconstant_custom_func

c25fdd8

update stump and test_stump

890e42e

add param custom_func to naive rolling_isconstant

807f15d

add an example for isconstant custom func to naive

40cd400

add test function for isconstant custom func

186fccb

add test function for isconstant custom function

3ac00ef

update minimum version of black

6aa504b

fix docstrings

35a280b

fix format

58c6f96

NimaSarajpoor added 8 commits March 5, 2023 15:26

avoid random behavior of argsort when values are the same by passing …

74e1637

…kind

Merge branch 'main' into subseq_constant_in_API

19226f8

Merge branch 'main' into subseq_constant_in_API

c8da3ef

update if block

b0374c3

update naive function rolling_isconstant

3f49999

add comment

5ca79ed

minor updates

11aa7c9

fix naive stump

353e4b2

seanlaw reviewed Mar 10, 2023

View reviewed changes

stumpy/core.py Outdated Show resolved Hide resolved

change function from public to private

4c9d5c5

NimaSarajpoor commented Mar 10, 2023

View reviewed changes

Merge branch 'main' into subseq_constant_in_API

5c386b7

NimaSarajpoor added 2 commits March 11, 2023 20:01

Merge branch 'main' into subseq_constant_in_API

992c669

fix coverage

f1519ea

seanlaw merged commit c06f0e9 into TDAmeritrade:main Mar 12, 2023

NimaSarajpoor deleted the subseq_constant_in_API branch September 4, 2023 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #695 Add "subseq_isconstant" param to API #789

Fix #695 Add "subseq_isconstant" param to API #789

NimaSarajpoor commented Jan 28, 2023

codecov-commenter commented Jan 28, 2023 •

edited

Loading

NimaSarajpoor commented Jan 28, 2023 •

edited

Loading

seanlaw commented Jan 29, 2023

NimaSarajpoor commented Jan 29, 2023 •

edited

Loading

seanlaw commented Jan 30, 2023 •

edited

Loading

NimaSarajpoor commented Feb 1, 2023

seanlaw left a comment

NimaSarajpoor commented Mar 10, 2023 •

edited

Loading

NimaSarajpoor commented Mar 10, 2023

seanlaw commented Mar 10, 2023

NimaSarajpoor commented Mar 10, 2023

NimaSarajpoor Mar 10, 2023

NimaSarajpoor Mar 12, 2023

seanlaw commented Mar 10, 2023

NimaSarajpoor commented Mar 10, 2023

seanlaw commented Mar 10, 2023

NimaSarajpoor commented Mar 10, 2023

seanlaw commented Mar 11, 2023

NimaSarajpoor commented Mar 11, 2023

NimaSarajpoor commented Mar 11, 2023

seanlaw commented Mar 11, 2023

NimaSarajpoor commented Mar 12, 2023

seanlaw commented Mar 12, 2023

Fix #695 Add "subseq_isconstant" param to API #789

Fix #695 Add "subseq_isconstant" param to API #789

Conversation

NimaSarajpoor commented Jan 28, 2023

codecov-commenter commented Jan 28, 2023 • edited Loading

Codecov Report

NimaSarajpoor commented Jan 28, 2023 • edited Loading

seanlaw commented Jan 29, 2023

NimaSarajpoor commented Jan 29, 2023 • edited Loading

seanlaw commented Jan 30, 2023 • edited Loading

NimaSarajpoor commented Feb 1, 2023

seanlaw left a comment

Choose a reason for hiding this comment

NimaSarajpoor commented Mar 10, 2023 • edited Loading

NimaSarajpoor commented Mar 10, 2023

seanlaw commented Mar 10, 2023

NimaSarajpoor commented Mar 10, 2023

NimaSarajpoor Mar 10, 2023

Choose a reason for hiding this comment

NimaSarajpoor Mar 12, 2023

Choose a reason for hiding this comment

seanlaw commented Mar 10, 2023

NimaSarajpoor commented Mar 10, 2023

seanlaw commented Mar 10, 2023

NimaSarajpoor commented Mar 10, 2023

seanlaw commented Mar 11, 2023

NimaSarajpoor commented Mar 11, 2023

NimaSarajpoor commented Mar 11, 2023

seanlaw commented Mar 11, 2023

NimaSarajpoor commented Mar 12, 2023

seanlaw commented Mar 12, 2023

codecov-commenter commented Jan 28, 2023 •

edited

Loading

NimaSarajpoor commented Jan 28, 2023 •

edited

Loading

NimaSarajpoor commented Jan 29, 2023 •

edited

Loading

seanlaw commented Jan 30, 2023 •

edited

Loading

NimaSarajpoor commented Mar 10, 2023 •

edited

Loading