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.

Sign up for GitHub

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

Jump to bottom

[ENH] Improve _get_series_len performance #2406

Merged
merged 2 commits into from
Apr 28, 2021
Merged

[ENH] Improve _get_series_len performance #2406

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1a50ab
Improve _get_series_len performance
HippocampusGirl Apr 28, 2021
bb9802b
Reference config object only init function
HippocampusGirl Apr 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion fmriprep/workflows/bold/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,10 @@ def _get_series_len(bold_fname):
if len(img.shape) < 4:
return 1

skip_vols = _get_vols_to_discard(img)
if isinstance(config.workflow.dummy_scans, int):
skip_vols = config.workflow.dummy_scans
else:
skip_vols = _get_vols_to_discard(img)
Copy link
Member

Choose a reason for hiding this comment

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

The config object is a hulking beast, and I'd prefer to keep its tentacles confined to init_*_wf() functions, and pass arguments to standalone functions, to make them easier to reason about.

def _get_series_len(bold_fname, skip_vols):
    from niworkflows.interfaces.registration import _get_vols_to_discard
    img = nb.load(bold_fname)
    if len(img.shape) < 4:
        return 1

    if skip_vols is None:
        skip_vols = _get_vols_to_discard(img)

    return img.shape[3] - skip_vols

And then, above, pass config.workflow.dummy_scans to _get_series_len().

Copy link
Member

Choose a reason for hiding this comment

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

We probably want to make an overhaul of the documentation, on the code guidelines section) and:

  • mention this (please do not use config deep in the workflow)
  • recommend black for everything except the workflow.connect function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't actually looked at the code guidelines in a while, so I read them again just now.

I realized that I probably also should have considered the branch naming/pull request naming, and added [ENH].


return img.shape[3] - skip_vols

Expand Down