From 244ad99900298d877a4b16e18e23c9f8bd962751 Mon Sep 17 00:00:00 2001 From: Remi Gau Date: Thu, 12 Nov 2020 08:41:30 +0100 Subject: [PATCH 1/2] make file moving function more robust and fail more gracefully --- src/reports/copyGraphWindownOutput.m | 31 +++++++++++++++-- src/utils/validationInputFile.m | 52 ++++++++++++++++++---------- tests/test_copyGraphWindownOutput.m | 28 +++++++++++++++ 3 files changed, 89 insertions(+), 22 deletions(-) diff --git a/src/reports/copyGraphWindownOutput.m b/src/reports/copyGraphWindownOutput.m index 809e4d4d..9e6d31b9 100644 --- a/src/reports/copyGraphWindownOutput.m +++ b/src/reports/copyGraphWindownOutput.m @@ -38,10 +38,30 @@ end for iFile = imgNb - + + % Using validationInputFile might be too agressive as it throws an error if + % it can't find a file. Let's use a work around and stick to warnings for now + + % file = validationInputFile(pwd, sprintf('spm_.*%i.png', iFile)); file = spm_select('FPList', pwd, sprintf('^spm_.*%i.png$', iFile)); - - if ~isempty(file) + + if isempty(file) + + warning(... + 'copyGraphWindownOutput:noFile', ... + 'No figure file to copy'); + + elseif size(file, 1) > 1 + + warning(... + 'copyGraphWindownOutput:tooManyFiles', ... + sprintf('\n %s\n %s\n %s', ... + 'Too many figure files to copy.', ... + 'Not sure what to do.', ... + 'Will skip this step.')); + disp(file) + + else targetFile = sprintf( ... '%s_%i_sub-%s_task-%s_%s.png', ... @@ -54,6 +74,11 @@ movefile( ... file, ... fullfile(figureDir, targetFile)); + + fprintf(1, '\n%s\nwas moved to\n%s\n', ... + file, ... + fullfile(figureDir, targetFile)); + end diff --git a/src/utils/validationInputFile.m b/src/utils/validationInputFile.m index debbc3d0..724f3d68 100644 --- a/src/utils/validationInputFile.m +++ b/src/utils/validationInputFile.m @@ -1,45 +1,59 @@ % (C) Copyright 2019 CPP BIDS SPM-pipeline developers -function files = validationInputFile(dir, fileName, prefix) +function files = validationInputFile(dir, fileNamePattern, prefix) % - % Short description of what the function does goes here. + % Looks for file name pattern in a given directory and returns all the files + % that match that pattern but throws an error if it cannot find any. + % + % A prefix can be added to the filename. + % + % This function is mostly used that a file exists so + % that an error is thrown early when building a SPM job rather than at run + % time. % % USAGE:: % - % [argout1, argout2] = templateFunction(argin1, [argin2 == default,] [argin3]) + % files = validationInputFile(dir, fileName[, prefix]) % - % :param argin1: (dimension) obligatory argument. Lorem ipsum dolor sit amet, - % consectetur adipiscing elit. Ut congue nec est ac lacinia. - % :type argin1: type - % :param argin2: optional argument and its default value. And some of the - % options can be shown in litteral like ``this`` or ``that``. - % :type argin2: string - % :param argin3: (dimension) optional argument + % :param dir: Directory where the search will be conducted. + % :type dir: string + % :param fileName: file name pattern. Can be a regular expression except for + % the starting ``^`` and ending ``$``. For example: + % ``'sub-.*_ses-.*_task-.*_bold.nii'``. + % :type fileName: string + % :param prefix: prefix to be added to the filename pattern. This can also be + % a regular expression (ish). For example ,f looking for the files that + % start with ``c1`` or ``c2`` or ``c3``, the prefix can be + % ``c[123]``. + % :type prefix: string % - % :returns: - :argout1: (type) (dimension) - % - :argout2: (type) (dimension) + % :returns: % - % file = validationInputFile(dir, fileName, prefix) + % :files: (string array) returns the fullpath file list of all the + % files matching the required pattern. + % + % + % See also: ``spm_select``. % - % Checks if files exist. A prefix can be added. The prefix allows for the - % use of regular expression. % - % TPMs = validationInputFile(anatDataDir, anatImage, 'c[12]'); + % Example: + % % + % % tissueProbaMaps = validationInputFile(anatDataDir, anatImage, 'c[12]'); % - % If the filet(s) exist(s), it returns a char array containing list of fullpath. + % if nargin < 3 prefix = ''; end - files = spm_select('FPList', dir, ['^' prefix fileName '$']); + files = spm_select('FPList', dir, ['^' prefix fileNamePattern '$']); if isempty(files) errorStruct.identifier = 'validationInputFile:nonExistentFile'; errorStruct.message = sprintf( ... 'This file does not exist: %s', ... - fullfile(dir, [prefix fileName '[.gz]'])); + fullfile(dir, [prefix fileNamePattern '[.gz]'])); error(errorStruct); end diff --git a/tests/test_copyGraphWindownOutput.m b/tests/test_copyGraphWindownOutput.m index 4fe8b6a4..1d6d3e36 100644 --- a/tests/test_copyGraphWindownOutput.m +++ b/tests/test_copyGraphWindownOutput.m @@ -7,6 +7,8 @@ end function test_copyGraphWindownOutputBasic() + + delete('*.png') opt.derivativesDir = pwd; opt.taskName = 'testTask'; @@ -33,3 +35,29 @@ function test_copyGraphWindownOutputBasic() rmdir(fullfile(opt.derivativesDir, ['sub-' subID]), 's'); end + + +function test_copyGraphWindownOutputWarning() + + delete('*.png') + + opt.derivativesDir = pwd; + opt.taskName = 'testTask'; + subID = '01'; + action = 'testStep'; + + system('touch spm_002.png'); + system('touch spm_test_002.png'); + + copyGraphWindownOutput(opt, subID, action, 2) + + assertWarning(... + @()copyGraphWindownOutput(opt, subID, action, 2), ... + 'copyGraphWindownOutput:tooManyFiles'); + assertWarning(... + @()copyGraphWindownOutput(opt, subID, action, 3), ... + 'copyGraphWindownOutput:noFile'); + + rmdir(fullfile(opt.derivativesDir, ['sub-' subID]), 's'); + +end From 8582318b464f41b35e892341fc70d1d5ef367dcd Mon Sep 17 00:00:00 2001 From: Remi Gau Date: Sun, 29 Nov 2020 18:03:52 +0100 Subject: [PATCH 2/2] add work around to prevent crashes in CI + mh fix --- src/reports/copyGraphWindownOutput.m | 43 ++++++++++++++-------------- tests/test_copyGraphWindownOutput.m | 41 +++++++++++++++++--------- 2 files changed, 48 insertions(+), 36 deletions(-) diff --git a/src/reports/copyGraphWindownOutput.m b/src/reports/copyGraphWindownOutput.m index 9e6d31b9..a7821cd0 100644 --- a/src/reports/copyGraphWindownOutput.m +++ b/src/reports/copyGraphWindownOutput.m @@ -32,35 +32,35 @@ action = ''; end - figureDir = fullfile(opt.derivativesDir, ['sub-' subID], 'figures'); + figureDir = fullfile(opt.derivativesDir, strcat('sub-', subID), 'figures'); if ~exist(figureDir, 'dir') mkdir(figureDir); end for iFile = imgNb - + % Using validationInputFile might be too agressive as it throws an error if % it can't find a file. Let's use a work around and stick to warnings for now - + % file = validationInputFile(pwd, sprintf('spm_.*%i.png', iFile)); file = spm_select('FPList', pwd, sprintf('^spm_.*%i.png$', iFile)); - + if isempty(file) - - warning(... - 'copyGraphWindownOutput:noFile', ... - 'No figure file to copy'); - + + warning( ... + 'copyGraphWindownOutput:noFile', ... + 'No figure file to copy'); + elseif size(file, 1) > 1 - - warning(... - 'copyGraphWindownOutput:tooManyFiles', ... - sprintf('\n %s\n %s\n %s', ... - 'Too many figure files to copy.', ... - 'Not sure what to do.', ... - 'Will skip this step.')); - disp(file) - + + warning( ... + 'copyGraphWindownOutput:tooManyFiles', ... + sprintf('\n %s\n %s\n %s', ... + 'Too many figure files to copy.', ... + 'Not sure what to do.', ... + 'Will skip this step.')); + disp(file); + else targetFile = sprintf( ... @@ -74,11 +74,10 @@ movefile( ... file, ... fullfile(figureDir, targetFile)); - - fprintf(1, '\n%s\nwas moved to\n%s\n', ... - file, ... - fullfile(figureDir, targetFile)); + fprintf(1, '\n%s\nwas moved to\n%s\n', ... + file, ... + fullfile(figureDir, targetFile)); end diff --git a/tests/test_copyGraphWindownOutput.m b/tests/test_copyGraphWindownOutput.m index 1d6d3e36..0958cc5a 100644 --- a/tests/test_copyGraphWindownOutput.m +++ b/tests/test_copyGraphWindownOutput.m @@ -7,8 +7,10 @@ end function test_copyGraphWindownOutputBasic() - - delete('*.png') + + delete('*.png'); + + pause(1); opt.derivativesDir = pwd; opt.taskName = 'testTask'; @@ -32,14 +34,19 @@ function test_copyGraphWindownOutputBasic() assertEqual(size(files, 1), 2); pause(1); + + if isOctave() + confirm_recursive_rmdir (true, 'local'); + end rmdir(fullfile(opt.derivativesDir, ['sub-' subID]), 's'); end - function test_copyGraphWindownOutputWarning() - - delete('*.png') + + delete('*.png'); + + pause(1); opt.derivativesDir = pwd; opt.taskName = 'testTask'; @@ -49,15 +56,21 @@ function test_copyGraphWindownOutputWarning() system('touch spm_002.png'); system('touch spm_test_002.png'); - copyGraphWindownOutput(opt, subID, action, 2) - - assertWarning(... - @()copyGraphWindownOutput(opt, subID, action, 2), ... - 'copyGraphWindownOutput:tooManyFiles'); - assertWarning(... - @()copyGraphWindownOutput(opt, subID, action, 3), ... - 'copyGraphWindownOutput:noFile'); - + copyGraphWindownOutput(opt, subID, action, 2); + + if ~isOctave() + assertWarning( ... + @()copyGraphWindownOutput(opt, subID, action, 2), ... + 'copyGraphWindownOutput:tooManyFiles'); + + assertWarning( ... + @()copyGraphWindownOutput(opt, subID, action, 3), ... + 'copyGraphWindownOutput:noFile'); + end + + if isOctave() + confirm_recursive_rmdir (true, 'local'); + end rmdir(fullfile(opt.derivativesDir, ['sub-' subID]), 's'); end