Fixed #966 Add Named Attributes to Matrix Profile

[seanlaw]

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!

• Fork, clone, and checkout the newest version of the code
• Create a new branch
• Make necessary code changes
• Install black (i.e., python -m pip install black or conda install -c conda-forge black)
• Install flake8 (i.e., python -m pip install flake8 or conda install -c conda-forge flake8)
• Install pytest-cov (i.e., python -m pip install pytest-cov or conda install -c conda-forge pytest-cov)
• Run black . in the root stumpy directory
• Run flake8 . in the root stumpy directory
• Run ./setup.sh && ./test.sh in the root stumpy directory
• Reference a Github issue (and create one if one doesn't already exist)

[seanlaw]

@NimaSarajpoor Would you mind reviewing this first draft?

[NimaSarajpoor]

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

[codecov]

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.

[seanlaw]

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

[seanlaw]

@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.

[seanlaw]

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

[NimaSarajpoor]

@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]

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.

[NimaSarajpoor]

@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]

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

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[seanlaw]

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]

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

[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
:::

[NimaSarajpoor]

@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]

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]

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]

@NimaSarajpoor I think it is ready for review again

[NimaSarajpoor]

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.

[NimaSarajpoor]

@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]

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

Nothing special except eventually honing in on the problem:

1. Search for jupyterlab-myst readthedocs
2. Realize that this isn't a jupyterlab problem anymore and search for myst-parser readthedocs
3. Realize that it's really about rendering .ipynb files in sphinx (this was key!) and then search for myst-nb sphinx
4. Realize that myst-nb should replace nbsphinx in the conf.py so continue searching results
5. 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!)
6. I tried blindly adding/removing various things from the conf.py until it worked and, boy, was it a frustrating process!
7. 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]

@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]

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]

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]

@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]

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]

@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]

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]

@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]

@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]

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]

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