Fixed #966 Add Named Attributes to Matrix Profile #972

seanlaw · 2024-05-16T02:22:50Z

See #966

Update docstrings (to add attributes)
Add unit tests
Update tutorials

Pull Request Checklist

Below is a simple checklist but please do not hesitate to ask for assistance!

seanlaw · 2024-05-16T02:23:56Z

@NimaSarajpoor Would you mind reviewing this first draft?

NimaSarajpoor · 2024-05-17T00:56:36Z

@seanlaw
Of course. Will do within next few days.

codecov · 2024-05-17T17:30:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.02%. Comparing base (f8f245c) to head (0c96429).
Report is 28 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #972      +/-   ##
==========================================
+ Coverage   98.93%   99.02%   +0.09%     
==========================================
  Files          84       85       +1     
  Lines       14293    14413     +120     
==========================================
+ Hits        14141    14273     +132     
+ Misses        152      140      -12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

NimaSarajpoor

@seanlaw
I provided a few comments.

stumpy/mparray.py

tests/test_mparray.py

stumpy/gpu_stump.py

stumpy/mparray.py

tests/test_mparray.py

stumpy/mparray.py

seanlaw · 2024-05-26T10:43:24Z

@NimaSarajpoor Thanks for reviewing. I have made some changes and added some comments

stumpy/mparray.py

stumpy/aamp.py

seanlaw · 2024-05-28T13:09:12Z

@NimaSarajpoor Do you think this is ready to be merged?

NimaSarajpoor

@seanlaw
Sorry for the late response. It is all good. I just added a few minor comments.

stumpy/mparray.py

stumpy/stump.py

seanlaw · 2024-06-02T15:32:10Z

@NimaSarajpoor Thanks for catching those. I've fixed them now

NimaSarajpoor · 2024-06-02T16:06:44Z

@seanlaw
I am able to merge the changes, but I want to ensure I don't appear to be overstepping my permissions 😅 Let me know if I should merge it myself (of course, after passing all tests)

seanlaw · 2024-06-02T16:17:00Z

Please go ahead and merge when you are satisfied with the changes 😊

NimaSarajpoor

@seanlaw
I missed this one (my bad!) I am going to take care of it.

stumpy/mparray.py

NimaSarajpoor · 2024-06-02T22:56:28Z

@seanlaw
I am about to merge. In the very first comment in this PR, I can see the checkbox Update tutorials is left unmarked. Did you initially have a plan to update tutorials in this PR?

seanlaw · 2024-06-03T00:27:22Z

Ooh, yes! Let me think about that and what to do

review-notebook-app · 2024-06-03T01:35:34Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

seanlaw · 2024-06-03T01:42:32Z

Ooh, yes! Let me think about that and what to do

Alright, @NimaSarajpoor what do you think of the tip/note that I've added as a colored box to the tutorials?

seanlaw · 2024-06-03T13:16:55Z

Frankly, I don't love how they look. I will try something else later today. Please stay tuned

seanlaw · 2024-06-03T14:07:17Z

Okay, adding the jupyterlab-myst extension allows us to render much nicer boxes using admonitions:

[comment]: <> (Myst)
:::{admonition} **Some title**
:class: note
Some body text

Add markdown code block here too
:::

NimaSarajpoor · 2024-06-03T23:50:37Z

@seanlaw

Okay, adding the jupyterlab-myst extension allows us to render much nicer boxes using admonitions:
[comment]: <> (Myst)
:::{admonition} **Some title**
:class: note
Some body text

Add markdown code block here too
:::

The box is indeed nicer. However, I initially saw the box in its un-rendered form, and I realized that I had to upgrade my notebook from 6 to 7 so that I can see the box.

Are you okay with this?
(Even when I click on the notebook link from the repo, the box doesn't seems to be rendered: https://github.com/seanlaw/stumpy/tree/Add_Named_Attributes/docs)

seanlaw · 2024-06-03T23:59:19Z

Are you okay with this?

I am seeing the same thing as you on Github and I am only able to render in Jupyterlab using the Myst extension. However, I am unable to get it to render for readthedocs (building the docs locally) so I am troubleshooting. There is little to no information on why it is failing to render the boxes

seanlaw · 2024-06-04T01:48:23Z

I think I figured it out. The key was adding myst_enable_extensions = ["colon_fence",] to conf.py since the admonition syntax required using a "colon_fence". Additionally, we had to replace nbsphinx with myst-nb in the extensions of conf.py. After this, I was able to build everything locally, which suggests that things should work for readthedocs

After switching to myst-nb, I also noticed that the code cells look stylized too!

seanlaw · 2024-06-04T03:09:48Z

@NimaSarajpoor I think it is ready for review again

NimaSarajpoor · 2024-06-04T05:37:11Z

I think I figured it out. The key was adding myst_enable_extensions = ["colon_fence",] to conf.py since the admonition syntax required using a "colon_fence". Additionally, we had to replace nbsphinx with myst-nb in the extensions of conf.py. After this, I was able to build everything locally, which suggests that things should work for readthedocs

After switching to myst-nb, I also noticed that the code cells look stylized too!

That was fast! You should share how you do it 😅

@NimaSarajpoor I think it is ready for review again

I will check the changes.

docs/Tutorial_AB_Joins.ipynb

docs/Tutorial_Fast_Approximate_Matrix_Profiles.ipynb

docs/Tutorial_Matrix_Profiles_For_Streaming_Data.ipynb

docs/Tutorial_STUMPY_Basics.ipynb

NimaSarajpoor · 2024-06-04T07:01:35Z

@seanlaw
I was wondering if there is a way to fix the updated notebooks (see link below) so that they can show the rendered box properly. https://github.com/seanlaw/stumpy/tree/Add_Named_Attributes/docs

If most of the users go and check the tutorials available in the readthedocs, then we should be okay. What do you think?

seanlaw · 2024-06-04T10:30:07Z

That was fast! You should share how you do it 😅

Nothing special except eventually honing in on the problem:

Search for jupyterlab-myst readthedocs
Realize that this isn't a jupyterlab problem anymore and search for myst-parser readthedocs
Realize that it's really about rendering .ipynb files in sphinx (this was key!) and then search for myst-nb sphinx
Realize that myst-nb should replace nbsphinx in the conf.py so continue searching results
Eventually, after quite a bit of time, poked around in the myst-nb Github repo and documentation and realize the notebooks are rendering using myst-nb but somehow the admonition boxes were the only thing NOT rendering so I looked at the docs a little bit more carefully and, in particular, blindly focus on the conf.py file (though I still didn't know if this was the problem!)
I tried blindly adding/removing various things from the conf.py until it worked and, boy, was it a frustrating process!
Ultimately, the colon_fence thing was the culprit and I have no idea how I figured it out as the docs were not actually direct/helpful to get there but I went to the myst-nb docs and stumbled from

myst-nb docs > authoring > Jupyter notebooks > Syntax exntensions > Colon Fencing

I'm sure there was a lot of branching along the way and it was very, very frustrating that I considered giving up numerous times!

seanlaw · 2024-06-04T10:36:09Z

@NimaSarajpoor Thanks for catching those copy/paste typos! I am doing too much at the same time and multitasking is killing me 😄. It's actually really, really nice to have somebody else reviewing with an eye for detail so thank you!

seanlaw · 2024-06-04T10:48:42Z

I was wondering if there is a way to fix the updated notebooks (see link below) so that they can show the rendered box properly. https://github.com/seanlaw/stumpy/tree/Add_Named_Attributes/docs

I had the same feeling but this is basically asking, "how do we force Github to render the .ipynb files using the myst-parser" and I am not sure. One may post a question on the myst-parser or myst-nb Github issues to ask? I don't know if it's worth it.

I think we are at the "early adopter" problem where it will take some time before anybody can get Github to change and add myst support. I just hope that readthedocs doesn't break this and we have to revert the changes to our tutorial

I posted a myst-parder discussion question here.

If most of the users go and check the tutorials available in the readthedocs, then we should be okay. What do you think?

Yes, I agree.

NimaSarajpoor · 2024-06-05T03:44:18Z

Nothing special except eventually honing in on the problem
I'm sure there was a lot of branching along the way and it was very, very frustrating that I considered giving up numerous times!

Thanks for sharing!! Great problem-solving skill! I think we all consider giving up from time to time.... but, eventually, we just keep going :)

I was wondering if there is a way to fix the updated notebooks (see link below) so that they can show the rendered box properly. https://github.com/seanlaw/stumpy/tree/Add_Named_Attributes/docs

I had the same feeling but this is basically asking, "how do we force Github to render the .ipynb files using the myst-parser" and I am not sure. One may post a question on the myst-parser or myst-nb Github issues to ask? I don't know if it's worth it.

I think we are at the "early adopter" problem where it will take some time before anybody can get Github to change and add myst support. I just hope that readthedocs doesn't break this and we have to revert the changes to our tutorial

I posted a myst-parder discussion question here.

Got it! Thanks for posting the question.

NimaSarajpoor · 2024-06-05T04:25:34Z

@seanlaw
I need to learn to slow down and review my own review (before submitting it) so that I make sure I am not missing anything! (I missed the typo in the notebook Tutorial_Semantic_Segmentation.ipynb. The latest commit should fix it. )

seanlaw · 2024-06-05T10:37:41Z

Thanks, @NimaSarajpoor. It looks like all of the tests are passing except that the codecov/patch is getting stuck on In progress for over 5 hours. I've never encountered that before

NimaSarajpoor · 2024-06-05T12:08:35Z

@seanlaw

except that the codecov/patch is getting stuck on In progress for over 5 hours

Can you let me know where I can find that? I took a look at the previous runs here: https://github.com/TDAmeritrade/stumpy/actions

And, I took a look at my first attempt:
https://github.com/TDAmeritrade/stumpy/actions/runs/9378123078/attempts/1

But couldn't locate the 5-hour delay (maybe it was not counted when it was in progress??).

As a side note:
I do however see some annotations errors. I can see it in previous runs as well even when the tests were passing. According to the provided message, we can add python3.8 -m pip install --upgrade pip to the Github Actions (or, maybe we just ignore it? But better to be addressed if possible)

As another side note:
I can see an error in the unit testing of snippet. The test for test_mpdist_snippets_s_with_isconstant[3-3-8-T0] failed. If you do not mind, I can take a look at this as I worked on snippet before.

seanlaw · 2024-06-05T14:40:11Z

Can you let me know where I can find that? I took a look at the previous runs here:

It's resolved now:

It was showing "In Progress" previously but the new commit seems to have fixed it

I do however see some annotations errors. I can see it in previous runs as well even when the tests were passing. According to the provided message, we can add python3.8 -m pip install --upgrade pip to the Github Actions (or, maybe we just ignore it? But better to be addressed if possible)

Please create a new issue for this so that it can be tracked separately

I can see an error in the unit testing of snippet. The test for test_mpdist_snippets_s_with_isconstant[3-3-8-T0] failed. If you do not mind, I can take a look at this as I worked on snippet before.

Let's handle this in a new issue as well please. Once you can confirm that you are able to reproduce the error and add it to the new issue then I will re-run the tests above so that we can close out this current issue/PR

NimaSarajpoor · 2024-06-06T02:38:32Z

@seanlaw

Once you can confirm that you are able to reproduce the error and add it to the new issue then I will re-run the tests above so that we can close out this current issue/PR

Please allow me some time to work on reproducing the error. I will get back to you

NimaSarajpoor · 2024-06-08T11:07:53Z

@seanlaw

Once you can confirm that you are able to reproduce the error and add it to the new issue then I will re-run the tests above so that we can close out this current issue/PR

Please allow me some time to work on reproducing the error. I will get back to you

I reported a similar failure in #984

seanlaw · 2024-06-08T15:19:21Z

I reported a similar failure in #984

Thank you. I am re-running the tests now. If they pass and there are no more changes, please feel free to squash and merge at your earliest convenience.

NimaSarajpoor · 2024-06-08T21:25:35Z

@seanlaw
Thanks for including me in this review! Going to merge it now.

Initial test of mparray class

f196cb1

seanlaw requested a review from NimaSarajpoor May 16, 2024 02:23

seanlaw added this to the Python 1.12.1/1.13.0 Release milestone May 16, 2024

Added unit tests and fixed mparray initialization issues

9ad9528

seanlaw added 2 commits May 17, 2024 14:15

Updated function docstrings using mparray

b8b6df9

Added docstring examples using named attributes

854a91e

NimaSarajpoor reviewed May 25, 2024

View reviewed changes

seanlaw added 3 commits May 25, 2024 17:43

Fixed several typos

cc07af8

Kept T_A, T_B in unit tests

d821d1e

Added exclusion zone denominator

972e031

NimaSarajpoor reviewed May 26, 2024

View reviewed changes

stumpy/mparray.py Outdated Show resolved Hide resolved

stumpy/aamp.py Show resolved Hide resolved

Fixed typo

43b8aa8

NimaSarajpoor reviewed Jun 2, 2024

View reviewed changes

stumpy/mparray.py Outdated Show resolved Hide resolved

stumpy/mparray.py Outdated Show resolved Hide resolved

stumpy/mparray.py Outdated Show resolved Hide resolved

stumpy/mparray.py Outdated Show resolved Hide resolved

stumpy/stump.py Show resolved Hide resolved

Fixed minor details

18401fa

NimaSarajpoor reviewed Jun 2, 2024

View reviewed changes

stumpy/mparray.py Show resolved Hide resolved

Add parameter's description to the docstring

f593bc4

Added tips/notes to notebooks

6117a5a

Reverted tutorials that only had metadata changes

db2b4e9

Added better text boxes with Myst

2b508c7

seanlaw added 2 commits June 3, 2024 21:44

Replaced nbsphinx with myst-nb

7fb3377

Fixed black formatting issue

47593c3

NimaSarajpoor reviewed Jun 4, 2024

View reviewed changes

docs/Tutorial_AB_Joins.ipynb Show resolved Hide resolved

docs/Tutorial_Fast_Approximate_Matrix_Profiles.ipynb Show resolved Hide resolved

docs/Tutorial_Matrix_Profiles_For_Streaming_Data.ipynb Show resolved Hide resolved

docs/Tutorial_STUMPY_Basics.ipynb Show resolved Hide resolved

Fixed typos

c070cb1

fixed typo

0c96429

NimaSarajpoor merged commit 31111f8 into TDAmeritrade:main Jun 8, 2024
35 checks passed

Fixed #966 Add Named Attributes to Matrix Profile #972

Fixed #966 Add Named Attributes to Matrix Profile #972

Conversation

seanlaw commented May 16, 2024 • edited by NimaSarajpoor Loading

Pull Request Checklist

seanlaw commented May 16, 2024

NimaSarajpoor commented May 17, 2024

codecov bot commented May 17, 2024 • edited Loading

Codecov Report

NimaSarajpoor left a comment

Choose a reason for hiding this comment

seanlaw commented May 26, 2024

seanlaw commented May 28, 2024

NimaSarajpoor left a comment

Choose a reason for hiding this comment

seanlaw commented Jun 2, 2024

NimaSarajpoor commented Jun 2, 2024 • edited Loading

seanlaw commented Jun 2, 2024

NimaSarajpoor left a comment

Choose a reason for hiding this comment

NimaSarajpoor commented Jun 2, 2024

seanlaw commented Jun 3, 2024

review-notebook-app bot commented Jun 3, 2024

seanlaw commented Jun 3, 2024

seanlaw commented Jun 3, 2024

seanlaw commented Jun 3, 2024 • edited Loading

NimaSarajpoor commented Jun 3, 2024 • edited Loading

seanlaw commented Jun 3, 2024 • edited Loading

seanlaw commented Jun 4, 2024 • edited Loading

seanlaw commented Jun 4, 2024

NimaSarajpoor commented Jun 4, 2024

NimaSarajpoor commented Jun 4, 2024

seanlaw commented Jun 4, 2024 • edited Loading

seanlaw commented Jun 4, 2024

seanlaw commented Jun 4, 2024 • edited Loading

NimaSarajpoor commented Jun 5, 2024

NimaSarajpoor commented Jun 5, 2024 • edited Loading

seanlaw commented Jun 5, 2024

NimaSarajpoor commented Jun 5, 2024 • edited Loading

seanlaw commented Jun 5, 2024

NimaSarajpoor commented Jun 6, 2024

NimaSarajpoor commented Jun 8, 2024

seanlaw commented Jun 8, 2024

NimaSarajpoor commented Jun 8, 2024

seanlaw commented May 16, 2024 •

edited by NimaSarajpoor

Loading

codecov bot commented May 17, 2024 •

edited

Loading

NimaSarajpoor commented Jun 2, 2024 •

edited

Loading

seanlaw commented Jun 3, 2024 •

edited

Loading

NimaSarajpoor commented Jun 3, 2024 •

edited

Loading

seanlaw commented Jun 3, 2024 •

edited

Loading

seanlaw commented Jun 4, 2024 •

edited

Loading

seanlaw commented Jun 4, 2024 •

edited

Loading

seanlaw commented Jun 4, 2024 •

edited

Loading

NimaSarajpoor commented Jun 5, 2024 •

edited

Loading

NimaSarajpoor commented Jun 5, 2024 •

edited

Loading