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

ENH: Document relevant steps #866

Merged
merged 9 commits into from
Mar 1, 2024
Merged

Conversation

larsoner
Copy link
Member

Before merging …

  • Changelog has been updated (docs/source/changes.md)

Closes #862

Examples:

Screenshot from 2024-02-29 14-59-38

Screenshot from 2024-02-29 14-57-21

(I didn't realize use_maxwell_filter had a direct impact on the CSP step but it does because we pick mag there if it's used!)

Draft because I need to parse a few functions for config defs but that's a tiny detail -- the general idea and code is otherwise reviewable.

@larsoner larsoner marked this pull request as ready for review February 29, 2024 20:14
@larsoner
Copy link
Member Author

Okay it was easier than I thought to parse the helper functions, ready for review/merge from my end @hoechenberger
!

@larsoner
Copy link
Member Author

larsoner commented Feb 29, 2024

Also with this in place we should maybe rethink some of our categorization, for example eog_channels is only used for artifact detection:

Screenshot from 2024-02-29 15-55-36

Maybe it should live in the artifacts section.

@larsoner larsoner added this to the 1.6 milestone Feb 29, 2024
@hoechenberger
Copy link
Member

I will take a look at this shortly

Thanks for your effort, Eric

{# START NEW CODE #}
{% if pipeline_steps(attribute_name) %}
{# https://squidfunk.github.io/mkdocs-material/reference/admonitions/#collapsible-blocks #}
<details class="success">
Copy link
Member

Choose a reason for hiding this comment

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

I haven't tried this, but I'd prefer to use "info" instead of "success"

Suggested change
<details class="success">
<details class="info">

Copy link
Member Author

Choose a reason for hiding this comment

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

I chose something other than info because that seemed to get a bit lost with the many other info blocks in our docs IIRC. Any others in here you like?

https://squidfunk.github.io/mkdocs-material/reference/admonitions/#collapsible-blocks

Copy link
Member Author

Choose a reason for hiding this comment

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

Like these:

$ git grep "[!?+]\+ info" mne_bids_pipeline/_config.py | wc -l
      26

So I wanted to differentiate / standardize using something else. Looks like note isn't used so I'll try that?

Copy link
Member

Choose a reason for hiding this comment

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

We can also add our own admonitions (what a word … I'll never remember it correctly!)

https://squidfunk.github.io/mkdocs-material/reference/admonitions/?h=ad#custom-admonitions

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure that would work, but we'd have to pick a color and icon. Maybe ✅ or 📋 icon? Actually abstract standard type (which we don't use anywhere) already uses the clipboard icon so that could be nice.

And maybe all the Good practice / advice that are currently info should be changed to tip?

Copy link
Member

Choose a reason for hiding this comment

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

Sure that would work, but we'd have to pick a color and icon. Maybe ✅ or 📋 icon? Actually abstract standard type (which we don't use anywhere) already uses the clipboard icon so that could be nice.

I would also suggest to use the current "Abstract" icon – not a custom unicode one.

But really it's something we can also adjust at any later time, it's not a blocker for me right now!

docs/templates/python/material/_base/attribute.html Outdated Show resolved Hide resolved
@larsoner
Copy link
Member Author

larsoner commented Mar 1, 2024

Okay I pushed with abstract, feel free to mark for merge-when-green if you're happy @hoechenberger !

@hoechenberger hoechenberger enabled auto-merge (squash) March 1, 2024 13:09
@hoechenberger hoechenberger merged commit 525646a into mne-tools:main Mar 1, 2024
51 of 52 checks passed
@larsoner larsoner deleted the where branch March 1, 2024 14:43
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.

DOC: Document steps where config options are used
2 participants