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

Update dccnpath #2282

Closed
wants to merge 17 commits into from
Closed

Update dccnpath #2282

wants to merge 17 commits into from

Conversation

contsili
Copy link
Collaborator

This PR is associated with issue #2281.

However, in dccnpath still two things need to be done (see: Todo notes in the dccnpath script):

  1. % Todo: FTP connection of private data: Download private data to the local copy from the DCCN cluster. This will apply only for employees and interns of the DCCN that have access to the DCCN intranet. I did not do that yet since I have no connection to the DCCN intranet. I will have connection after 1st of August.
  2. % Check if filename exists. If not, download it (Todo: after permission): Ask the permission of the user prior to downloading the data.

The test_dccnpath has been tested in Windows and it works. It should also be tested in Mac/Linux.

The list of DEPENDENCIES in the test_dccnpath are not accurate. They should be checked again. One question is how we are going to label the DATA DEPENDENCY. For now test_dccnpath only uses public data, but when % Todo: FTP connection of private data is completed it should also work on private data. My suggestion % DATA public private.

Help documentation still needs to be updated based on issue #2281.

Lastly, in dccnpath something that needs more attention is if ~exist(alternative2, 'file'). This checks if the file or folder exists. If not, it downloads them automatically. However, in the case of the folder it is important to check what files are missing from that folder and not generally if the folder is missing. Because if the folder exists but it is empty then nothing is downloaded while it should have.

contsili and others added 7 commits July 7, 2023 18:53
…tically download files and folders to the local copy, if they are not already downloaded. To download a whole folder, dccnpath uses the function utilities/private/recursiveDownload.m, 2. test_dccnpath is created to test the old and the newly added functionality
@contsili contsili added this to In progress in MATLAB Community Toolbox Training Projects 2023 via automation Jul 15, 2023
@contsili contsili marked this pull request as draft July 17, 2023 08:50
…. when ft_default.dccnpath is not specified the data are automatically downloaded to the tempdir 2. dccnpath() always outputs a path, even for alternative1 3. warning for private data and automatic errors given by webread for typos in the path.
@contsili
Copy link
Collaborator Author

contsili commented Jul 18, 2023

With the latest commit I added new features to dccnpath() and the corresponding tests in test_dccnpath(), as discussed in the meeting we had with @robertoostenveld on 17/07/2023.

Questions

  1. When ft_default.dccnpath is not defined and the data are downloaded in the tempdir, shall I add a warning('local path was not explicitly defined. Using tempdir to store the test data')? Already, dccnpath() prints at least one warning each time it runs. Maybe is better to not add another one. Anyhow, the path (tempdir in this case) where the data are downloaded is always printed after dccnpath() runs, so the user can see the path also without the warning.

  2. Contributors that have remote access to the test data via a mounted drive will use the alternative2 of the dccnpath() to find the right path. In this case they should define ft_default.dccnpath=your/local/copy as we explain in the help documentation of the dccnpath().

    Examples of such contributors are: contributors with access to the DCCN intranet that have mounted to their local computer the public and private test data or contributors with no access to the DCCN intranet that have mounted to their local computer the WebDAV server that includes only the public test data.

  3. For alternative1: I kept the old code:

elseif endsWith(pwd, alternative1)
% exist(alternative1, 'file') also returns 7 in case the present working directory name matches alternative1
% this is not the match we want
alternative1 = '';
end

Why are we using this chunk of code? If the contributor's current working directory in MATLAB is the one that the test data are downloaded, then why he should not be able to use alternative1 to find the right path?

  1. The error that is printed for typos in the path is:

    Error using matlab.internal.webservices.HTTPConnector/copyContentToByteArray
    The server returned the status 404 with message "Not Found" in response to the request to URL
    https://download.fieldtriptoolbox.org/test/ctf/Subject0.ds.

    This error is printed automatically by MATLAB webread function. This error appears when the data need to be downloaded, but also when the data do not need to be downloaded (since they already exist in the local copy). This happens because when the user does a typo to the path, the function always thinks that it is not downloaded (since the path is wrong) and tries to download. Is this solvable?

  2. When ft_default.dccnpath is given by the user, dccnpath() should always select alternative2 or should first check alternative0 and alternative1 and then if these do not exist go to alternative2? Right now it does the second.

  3. Finally, in dccnpath something that needs more attention is if ~exist(alternative2, 'file') || ~exist(alternative2, 'dir'). This checks if the file or folder exists. If not, it downloads them automatically. However, in the case of the folder it is important to check what files are missing from that folder and not generally if the folder is missing. Because if the folder exists but it is empty then nothing is downloaded while it should have. (I mentioned this also in my previous comment in this PR, but it is important to see if this might cause problems when trying to find the right path with dccnpath())

@robertoostenveld robertoostenveld added the summer project MATLAB for Neuroscience Summer Project label Aug 16, 2023
@robertoostenveld robertoostenveld linked an issue Aug 17, 2023 that may be closed by this pull request
…ataFolder did not exist. On the contrary, when I added a '/' in the end of the path, i.e., dccnpath(path/to/testdataFolder/) was not giving an error even if testdataFolder did not exist.
…ath outline given in ft_default.dccnpath. Based on this, test_dccnpath was updated accordingly
@contsili contsili marked this pull request as ready for review August 18, 2023 10:38
@contsili contsili marked this pull request as draft August 19, 2023 09:47
@contsili contsili marked this pull request as ready for review August 22, 2023 16:23
@contsili
Copy link
Collaborator Author

contsili commented Aug 22, 2023

The dccnpath and test_dccnpath are ready for review @robertoostenveld.

Two issues I want to highlight:

  1. dccnpath cannot find the present working path when we call dccnpath('/home/common/matlab/fieldtrip/data/ftp/test/ctf/Subject01.ds/BadChannels’) because BadChannels is a file with no file extension, so matlab thinks it is a folder.

  2. dccnpath & test_dccnpath can use fprintf() or ft_notice() to print more nicely their outputs. Right now dccnpath() uses the MATLAB command warning().

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.

Update dccnpath and add test_dccnpath
2 participants