Skip to content

added acq_details options - #211

Merged
tiborauer merged 2 commits into
automaticanalysis:masterfrom
jones-michael-s:BIDS_extensions
May 3, 2020
Merged

added acq_details options#211
tiborauer merged 2 commits into
automaticanalysis:masterfrom
jones-michael-s:BIDS_extensions

Conversation

@jones-michael-s

Copy link
Copy Markdown
Collaborator

This is a simple one-file PR -- I've added some processing flags to aas_processBIDS, tweaked string processing in one place to fix a bug when one event name is contained in another, and done some cosmetic clean up.

@jones-michael-s

Copy link
Copy Markdown
Collaborator Author

I have two rather substantial PRs ready to go after this (one includes modification of all the modeling modules, and the other is a half-dozen new modules to implement motion scrubbing).

At first I though submitting these as three smaller PR was a good approach (hence this one-file PR of our BIDS tweaks) but in hindsight maybe this is a bad idea? It occurs to me I might be tripling the number of times the test suite has to get run.

If there's a consensus that one big PR would make testing easier, I could do that instead.

Any thoughts?
Mike

@tiborauer

Copy link
Copy Markdown
Member

Although, there are some practicality in submitting one PR, I also think independent substantial changes deserve separate PRs.
If these changes affect well-defined areas in the code and well-specified use-cases, then we can test them separately - even in parallel.

@tiborauer

Copy link
Copy Markdown
Member

I can see a few useful settings, however, it is not clear from the beginning what are the default settings. Since they are not part of the app_paremetersets. I can also see that backward compatibility is ensured by using isfield(...), you may be able to hit two birds with one stone, if you would define defaults settings at the beginning (and in one place), if they are not specified. Similarly to

if ~exist('sessnames','var') || isempty(sessnames)
sessnames = [];
end
if ~exist('tasknames','var') || isempty(tasknames)
tasknames = [];
end
if ~exist('regcolumn','var') || isempty(regcolumn)
regcolumn = 'trial_type';
end

@jones-michael-s

Copy link
Copy Markdown
Collaborator Author

Okay, I added initialization that hopefully clarifies the defaults.

Tested locally; all looks good on my end...

@tiborauer
tiborauer merged commit 53ef81e into automaticanalysis:master May 3, 2020
@jones-michael-s
jones-michael-s deleted the BIDS_extensions branch June 4, 2021 16:55
tiborauer added a commit that referenced this pull request Jul 10, 2022
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.

2 participants