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

FIX - test_dccnpath to run in the DCCN cluster #2299

Merged
merged 1 commit into from Aug 24, 2023

Conversation

contsili
Copy link
Collaborator

@contsili contsili commented Aug 23, 2023

This PR is related to #2298 and contsili#6.

For now, the dccnpath we have developed works hierarchically. It first uses alternative0 and if the file exists then dccnpath stops (using the MATLAB command return). If alternative0 doesn't lead to a file that exists, then dccnpath uses alternative1, and finally alternative2 in the same way.

So, for all the DCCN cluster computers, alternative0 should always work since the files are pre-downloaded to these computers. As a result, test_dccnpath will not work properly when testing alternative1 and alternative2 inside the DCCN computer cluster.

I made corrections to test_dccnpath, so it will now work properly in the DCCN desktops and in the HPC cluster.

Note: another way to overcome this issue is to replace if ~startsWith(filename, tmpdir) with if ~exist(filename, 'file') / if ~exist(filename, 'dir') for alternative1 and alternative2-test1 in test_dccnpath. But in this case our if statement becomes a bit more general than what we try to test.

@robertoostenveld
Copy link
Contributor

If, on a compute node, I specify ft_default.dccnpath to be something different, then that must be used. Also, if on a compute node, the file exists in the present working directory, that file must be used. Either one might for example happen with a new or updated test for which the corresponding data has not yet been copied to the standard location.

So in those cases alternative0 is skipped and alternative1 or alternative2 are used.

@robertoostenveld
Copy link
Contributor

I agree that an extra check is in place prior to deleting the temporary directory. We don't want any intermediate/undetected change of ft_default.dccnpath to cause another directory to be deleted.

@robertoostenveld
Copy link
Contributor

I realize that alternative 0 takes precedence over 1 and 2, so the user is technically not able to specify on the compute cluster where the data is. I suppose that is what you mean...

I do think that the desired (i.e., perfect) behavior is what I described above: the user is in charge of determining which file is to be read. However, for now it is not important enough to rewrite dccnpath again to accommodate this, as an FT contributor who works on the DCCN cluster is likely to be me or @schoffelen anyway, and we know how it works and/or will find a workaround. So let's not make it perfect, I believe it is good enough already.

Your change to the test script (making it conditional on where it runs) is therefore appropriate.

@robertoostenveld robertoostenveld merged commit b1ec31d into fieldtrip:master Aug 24, 2023
1 check passed
MATLAB Community Toolbox Training Projects 2023 automation moved this from In progress to Done Aug 24, 2023
@robertoostenveld robertoostenveld added the summer project MATLAB for Neuroscience Summer Project label Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
summer project MATLAB for Neuroscience Summer Project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants