Skip to content

Fix outdated Tutorial_DiscordMERLIN.ipynb - #1157

Open
ShreyasK06 wants to merge 2 commits into
stumpy-dev:mainfrom
ShreyasK06:fix/tutorial-discord-merlin
Open

Fix outdated Tutorial_DiscordMERLIN.ipynb#1157
ShreyasK06 wants to merge 2 commits into
stumpy-dev:mainfrom
ShreyasK06:fix/tutorial-discord-merlin

Conversation

@ShreyasK06

Copy link
Copy Markdown

Pull Request Checklist

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

  • Read our Contributing Guide
  • Referenced a Github issue (or create one if one doesn't already exist)
  • Left a meaningful comment on the original Github issue to discuss the detailed approach for your contribution
  • Forked, cloned, and checkedout the newest version of the code
  • Created a new branch
  • Made necessary code changes
  • Installed black (i.e., python -m pip install black or conda install -c conda-forge black)
  • Installed flake8 (i.e., python -m pip install flake8 or conda install -c conda-forge flake8)
  • Installed pytest-cov (i.e., python -m pip install pytest-cov or conda install -c conda-forge pytest-cov)
  • Ran black --exclude=".*\.ipynb" --extend-exclude=".venv" --diff ./ in the root stumpy directory
  • Ran flake8 --extend-exclude=.venv ./ in the root stumpy directory
  • Ran ./setup.sh dev && ./test.sh in the root stumpy directory

Summary

Fixes #1127

This PR fixes docs/WIP/Tutorial_DiscordMERLIN.ipynb, which was broken in several ways due to API changes and dependency updates since it was originally written. All changes are scoped strictly to the notebook file.

Fix 1 — core.preprocess now returns 4 values
Updated every T, M_T, Σ_T = core.preprocess(...) call to the correct 4-value unpack (T, M_T, Σ_T, T_subseq_isconstant = core.preprocess(...)), and threaded T_subseq_isconstant through _find_candidates, _get_approx_P, and _refine_candidates so it is passed correctly to core._mass and _prescrump.

Fix 2 — Dead dataset URLs replaced

  • NoisySine dataset: original source is gone and no working URL exists. Replaced with a self-contained synthetic signal (sinusoid + noise + spike anomaly) generated with NumPy. No new dependencies required.
  • NYC taxi dataset: replaced broken local path with the Zenodo URL provided by the maintainer (https://zenodo.org/record/4276428/files/STUMPY_Basics_Taxi.csv?download=1).

Fix 3 — core._sliding_dot_product renamed
Updated both call sites in _find_candidates and _refine_candidates from core._sliding_dot_product to core.sliding_dot_product.

Fix 4 — np.NINF removed in NumPy 2.0
Replaced all 8 runtime occurrences of np.NINF across _refine_candidates, the MP verification block, _discords, and stumpy_top_k_discords with -np.inf. Occurrences in docstrings and comments were left untouched.

Known issue — pending review discussion
The notebook still fails at out = _discords(T, m, k=15) with:
ValueError: could not broadcast input array from shape (1489,1489) into shape (1489,)
This is caused by stumpy.scrump._prescrump now returning a 2D (l, k) array instead of the 1D (l,) shape _get_approx_P expects. Fix is pending maintainer input.

@ShreyasK06
ShreyasK06 requested a review from seanlaw as a code owner July 20, 2026 16:56
@gitnotebooks

gitnotebooks Bot commented Jul 20, 2026

Copy link
Copy Markdown

Found 1 changed notebook. Review the changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1157

@seanlaw
seanlaw requested review from NimaSarajpoor and removed request for seanlaw July 21, 2026 00:40
@NimaSarajpoor

Copy link
Copy Markdown
Collaborator

@ShreyasK06
Thanks for submitting this PR. Please allow me a couple of days to go through it and share my feedback.

@NimaSarajpoor NimaSarajpoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShreyasK06
Thanks again for submitting this PR. I've reviewed the changes till a certain point and shared a few comments for your consideration. Once those comments are discussed/addressed, I can review again and provide comments for the rest of the notebook.

Note:
Better to use the link provided here to review the provided comments. I've shared the link below for your convenience.

https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1157

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this part (including the link) as the link is broken and the data is now created here in this notebook.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be changed to Create toy data

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you run the cell to update figure?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description for T_subseq_isconstant is missing

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you run the cells to see the updated output here? If not, please do so and revise the description here if needed.

@ShreyasK06

Copy link
Copy Markdown
Author

Thanks for the review comments — I've addressed all four:

Removed the outdated dataset reference and broken link from the markdown cell.
Renamed the heading to Create toy data.
Added the missing T_subseq_isconstant docstring entry to _find_candidates, _get_approx_P, and _refine_candidates. Note that _discords and the public discords() wrapper don't take T_subseq_isconstant as a parameter since they compute it internally via core.preprocess, so no docstring edit was needed there.
Reviewed all other markdown cells — the only other dataset mention refers to the Zenodo taxi data example later in the notebook, which is still accurate and was left untouched.

One thing I wasn't able to do yet is re-run the cells to update the figure outputs, because the notebook still fails at out = _discords(T, m, k=15) with the _prescrump shape mismatch flagged earlier in the issue.

@NimaSarajpoor

NimaSarajpoor commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

One thing I wasn't able to do yet is re-run the cells to update the figure outputs, because the notebook still fails out = _discords(T, m, k=15) with ...

The goal is not to run the full notebook. If you make changes till a certain point... did you try to run cells till that point... or all those cells were failing till that point?


Thanks for the review comments — I've addressed all four:

If a certain comment is addressed, you can add a response in that thread to provide an update or just resolve it by pushing the button "Resolve Conversation". If a certain comment needs further discussion, we can keep the conversation there. For instance, in one comment, I said "Did you run the cell to update figure?". If you notice any issue regarding this comment, we can keep relevant conversation in that thread to keep this PR clean. Hope that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs/WIP/Tutorial_DiscordMERLIN.ipynb is outdated

2 participants