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

DOC: Automatic flowchart #860

Merged
merged 9 commits into from
Feb 26, 2024
Merged

DOC: Automatic flowchart #860

merged 9 commits into from
Feb 26, 2024

Conversation

larsoner
Copy link
Member

Before merging …

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

Locally produces:

Screenshot from 2024-02-23 14-52-05

@hoechenberger feel free to push directly wording changes you want. Locally you can iterate pretty easily with:

~/python/mne-bids-pipeline/docs/site$ python -m http.server
~/python/mne-bids-pipeline/docs/site$ ../build_docs.sh

then refreshting http://localhost:8000/features/overview.html in your browser.

@larsoner
Copy link
Member Author

@SophieHerbst can you see if this fixes the wording issue(s) you ran into or if there is more to do?

@@ -224,7 +224,7 @@ def apply_ica_raw(
raw_fname = in_files.pop(in_key)
assert len(in_files) == 0, in_files
out_files = dict()
out_files[in_key] = raw_fname.copy().update(processing="clean")
out_files[in_key] = raw_fname.copy().update(processing="clean", split=None)
Copy link
Member

@hoechenberger hoechenberger Feb 23, 2024

Choose a reason for hiding this comment

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

Should we check all code that generates output filenames to ensure we set split=None everywhere?

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 think we could do it in the function that prepares the output filenames actually. I'll give it a shot

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be working now!

@SophieHerbst
Copy link
Collaborator

@larsoner is the 'temporal regression for artifact removal' a default step? I am not aware that I am using it.
If not, I think it should be marked as optional.

@hoechenberger
Copy link
Member

most of the steps are optional though.... we'd end up with almost everything being marked as such

@SophieHerbst
Copy link
Collaborator

mh right. I am wondering how to make it clearer which config options trigger which steps (or their re-run)

@hoechenberger
Copy link
Member

there are get_config() functions in every step now which cover almost all configuration options that affect this step (exceptions are I believe n_jobs, subjects and session, but i'm not 100% certain from the top of my head)

in theory this information could be used

@SophieHerbst
Copy link
Collaborator

Used as in informing the documentation?
(I learned about this part last week, when I forgot to mention the image_kwargs for the report there; easy to make mistakes as it needs to be added manually)

@hoechenberger
Copy link
Member

Used as in informing the documentation?

Yes

We'd basically have to iterate over all steps and grep the arguments used in get_config() and then put that in a table or so…

@larsoner
Copy link
Member Author

Opened #862 and #861 so we can iterate further in follow-up PRs. This one in the meantime is 90% a bugfix for missing / incorrect information at the moment (the other 10% being the ICA/SSP enhancement to try and make that part clearer) -- @SophieHerbst @hoechenberger happy with those parts so we can merge this?

@hoechenberger
Copy link
Member

yes, this is great! feel free to merge whenever you feel it's ready 👍👍👍

@larsoner larsoner merged commit 61b3553 into mne-tools:main Feb 26, 2024
54 checks passed
@larsoner larsoner deleted