Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Control example order via explicit list, not via sorting titles #1321

Merged
merged 6 commits into from
Nov 11, 2024

Conversation

drammock
Copy link
Member

PR Description

This PR removes numbering from the titles of the examples, and switches the example ordering heuristic from relying on the (no-longer-numbered) titles, to instead relying on a JSON file containing example filenames in a fixed order. Motivation is that:

  • there were two examples numbered 13
  • the second of the "13"s seemed out of place at the end, but inserting it in the middle would have required renumbering all titles that came after its new place.

Safeguards:

  • If a new example is created and not added to the JSON file, it will automatically sort last (not be omitted from the build).
  • If an example's filename changes without updating the JSON file, it will sort last (not be omitted from the build).
  • Recovering from either of the above situations is a one-line change to the JSON file (i.e. adding the new filename / updating the old filename to its new name).

closes #1320

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

  • All comments are resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • New contributors have been added to CITATION.cff
  • PR description includes phrase "closes <#issue-number>"

Copy link
Member Author

@drammock drammock left a comment

Choose a reason for hiding this comment

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

quick self-review to call out / fix a couple things

"rename_brainvision_files.py",
"convert_mri_and_trans.py",
"convert_ieeg_to_bids.py",
"convert_nirs_to_bids.py",
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the one that was previously a duplicate "13" and appearing last. I thought it made more sense to group it with other "converting XXX to BIDS" examples.

Aside: if anyone has strong feelings about order / wants to take this opportunity to shuffle the order a bit more, feel free to push a commit changing this file.

Comment on lines 522 to 524
# better example sorting, without relying on numbers in example titles
with open(Path(__file__).parents[1] / "doc" / "example_order.json") as fid:
EXAMPLE_ORDER = json.load(fid)
Copy link
Member Author

Choose a reason for hiding this comment

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

NB: the JSON file won't be included in the built wheel, but that shouldn't matter as it's only needed when building our docs, so we can assume a clone of the repo in that case.

mne_bids/utils.py Outdated Show resolved Hide resolved
mne_bids/utils.py Outdated Show resolved Hide resolved
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Great, thanks a lot Dan. I like the changes, +1 to merge once CIs are green.

@drammock
Copy link
Member Author

just a note that this needs sphinx-gallery/sphinx-gallery#1391 before the CIs will pass.

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 97.42%. Comparing base (2b9b8cc) to head (2ce871d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
mne_bids/utils.py 28.57% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1321      +/-   ##
==========================================
- Coverage   97.47%   97.42%   -0.06%     
==========================================
  Files          40       40              
  Lines        8876     8883       +7     
==========================================
+ Hits         8652     8654       +2     
- Misses        224      229       +5     

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

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

@drammock I pushed a commit making sphing_gallery master a temporary dependency (until their next release). Tests seem to pass and everything is looking good. Thanks for your work!

Was there anything you wanted to add? If not, please feel free to merge!

@drammock drammock merged commit 81d8afd into mne-tools:main Nov 11, 2024
21 of 23 checks passed
@drammock drammock deleted the example-numbering branch November 11, 2024 19:50
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.

there are 2 examples numbered "13"
2 participants