GenPath Tool Cleanup Pull Request

[dankessler]

This represents some work on the GeneratePath "toolbox".

• It's now a single file: GeneratePath.m
• The only argument it needs is a template. It will parse out the "variables" and replace them with the values of the variables in the calling environment
• It takes optional argument of mode, which if 'check' will check if the path exists, and if not, notify the user in a "friendly" way a la Friendly Error Handling #11. If 'make' it will make the directory GeneratePath built.
• Code is now written in cell mode to make it more readable
• Several comments added to clarify script
• Cleaned up some nastiness at bottom of script that was leftover from previous merge conflict.

This should close #22.

It'd be great if @mangstad and @sripada could sign off on this. Once it's integrated, @shijiaguo may want to rebase her dcmbatch work related to #14 to take advantage of the GeneratePath tools.

[dankessler]

PS if we do accept this pull request, some of the template scripts may need to be rewritten so that they directly call GeneratePath rather than the older, now unsupported eval-ed GeneratePathCommand approach. That will merit opening another issue, but I'll hold off on that until the pull request gets accepted.

[sripada]

@mangstad and @sripada @shijiaguo @dankessler

What happened to pizza=1? That was a critical piece of code! Please put that back.

Seriously, this is awesome!! Love the new code. I definitely sign off on it.

One perhaps small issue: I thought you were going to makedir recursively. Looks like the current one will fail if you try to make a directory inside a directory that itself needs to be made.

[dankessler]

Actually, @sripada, pizza=1 does still exist :) It just moved :P

[dankessler]

@sripada More seriously though, in testing, mkdir will work recursively, regardless of whether you specify a path relative to pwd, or an absolute path. The only time it fails, and unfortunately with a kind of ugly message, if is you to try to mkdir into a path that you don't have write rights, to.

However, I'll add a commit that puts the mkdir in a try catch block so that if it fails, the user will get a more informative error dialog than seeing "path doesn't exist" or something blah like that.

[mangstad]

Yeah, I didn't realize this -- mostly due to the different nature of the default mkdir behavior in bash -- but mkdir in matlab does work recursively so should only fail if the user running the script doesn't have permission to some part of the path being created. Though I agree a more helpful error message would be appropriate.

[dankessler]

How does this new try/catch block look to you @sripada @mangstad ?

[dankessler]

I guess I could've put the comment at the bottom of the code I wanted you to check so that you could see it in the discussion panel. Instead, just click on the "view full changes" at the top right of this little element, and you'll zip to the lines in question.

[mangstad]

This exist will only try to create templatepath if it already exists as a file or directory, which doesn't seem to be the desired behavior.

[dankessler]

Another good catch. That should be ==0, not ~=0. Fixing it now.

[dankessler]

Ok, this is fixed now, though in editing it I destroyed our commentary in the diff, but this mini commentary appears to still be live.

[mangstad]

Also, we need to make sure that if there is no file at the end of OutputTemplate, that it ends in a /, otherwise fileparts will give you undesired behavior.

For example, fileparts('/root/directory/') will correctly give you /root/directory/ for templatepath and an empty string for templatename, but fileparts('/root/directory') will give you /root/ for templatepath and directory for templatename.

[dankessler]

Ok, I'll think about that and write a correction that will go through a sequence of checking if OutputTemplate is itself a directory, and only if not, passing templatename to mkdir.

There was also a problem with the check if path exists routine, as the logic was flipped there, too, in that you only want to trigger an error if it doesn't exist, e.g. == 0. That's already fixed, though.

[dankessler]

Ok, so I've been thinking about this, and it's more of a usage issue. These are going to be called by the centralscripts, not the templates, so it's more a matter of enforcement for the developers, not end-users.

If somebody specifies 'make' mode, they presumably are about to make a write operation. The path that GeneratePath returns may be either

1. A path to an actual filename that they plan to write out
2. A path to a directory that they're then going to write into, presumably using hardcoded or standardized filenames

Presumably if they are running in 'make' mode, in all likelihood, OutputTemplate points to something that doesn't exist. It's pretty hard for GeneratePath to know whether that thing that doesn't exist ought to be a directory or the file itself, in which case it needs to make a parent.

It may be easier to split this up into two modes, so that the call has to directly specify what it wants to happen.

1. makedir mode would make a directory with EXACTLY the same name as what genpath returns, so improper usage could result in a directory called rarun_01.nii
2. makeparentdir mode, which would always strip off the end of the path with fileparts, and make the parent directory.

It'd then be up to the developer who is calling GeneratePath to specify which mode they want to run in, rather than up to GeneratePath to try to read the mind of whoever is calling it.

What do we think of this, @sripada and @mangstad?

[sripada]

@mangstad , @dankessler

Is there a typo here. Should be OutputTemplate. Same problem in line 97. Or I could be missing something....

[dankessler]

Nice catch @sripada. I tested the function, but not the make and check options, which I'm doing now and just caught those errors. Sorry to have to go through so many iterations with you and @mangstad, but I appreciate the feedback. Another commit in a couple minutes should fix all these open issues.

[dankessler]

Ok, so I think I've restructured things (and tested them, too!) per Mike's issue with parsing a parent directory. Now there are two modes as described in one of the diff discussions above.

Any more thoughts, @mangstad or @sripada? Hopefully we can pull this in soon :)

[sripada]

@dankessler , @mangstad
maybe say '... problem writing this file: %s ...'

[dankessler]

Ok, I have updated this to say "problem writing this file/directory" because sometimes you might be trying to make a directory with this. Commits on their way.

[sripada]

@dankessler , @mangstad

I love it!

I made a few comments on the error messages.

If you want to go really gung ho, here is one more suggestion:
Let the script take one more optional argument for expected file suffix. This might be .csv or .nii etc... Then make the GeneratePath script Try the OutputTemplate with and without the suffix. The reason for this is we are having a hard time standardizing the Template documents. Sometimes you put the suffix in the template, other times you don't. This way, people can do either and the GeneratePath part takes care of it in the background.

Of note, the Try needs to be skipped if the template contains a wildcard, i.e., *

On second thought, this may be getting overly complicated. I will leave this up to you guys if you think there is merit in this idea.

[sripada]

Actually, the GeneratePath script doesn't need to Try anything. It just needs to check if the expected file type argument is at the end of the OutputTemplate, and if not, then it needs to append it. This may be worth doing as it is a frequent error that is easily correctable.

[mangstad]

Hey @dankessler could you give me a quick rundown of how I can pull your copy of this into my local repo so I can just run a few quick tests also? I swear eventually I'll get the hang of the git stuff.

[dankessler]

Sure:

git remote add dankessler git@github.com:dankessler/MethodsCore #This will add my repository as a remote
git ls-remote dankessler (this will give you a listing of all of my branches)
git fetch dankessler matlabScripts/GenPathTools:matlabScripts/GenPathTools #This will move my branch (refspec on left) to a new branch for you (refspec on right of colon)

You could also fetch ALL of my branches (as remote tracking branch) using...
git fetch dankessler

[dankessler]

Alright, I've mapped out the logic and all the switches on a whiteboard, and I think I finally have consistent behavior. Here is the sequence of processing. I've tried to take into account all kinds of bizarre edge cases.

1. Bracket expansion. We need to do this first in case one of their variables contains a wildcard, a suffix, or a final file seperator.
2. DirCheck: Only if type=1
   • Check if OutputTemplate ends in a file seperator. If not, append it.
   • Disable all suffix routines my clearing suffix variable. Thus, type takes precedence over suffix
3. SuffixCheck: Only if suffix exists
   • Check that OutputTemplate ends with suffix, and if not, append it.
   • If mode is set to 'makedir', disable it by setting mode to 'null'
4. Wildcard Checks
   1. If wildcards exist in "file" or "ext" parts, if mode is set to 'makedir', disable it by setting mode to 'null'
   2. If wildcards exist in lowest level directory, if mode is set to 'makeparentdir', disable it by setting mode to 'null'
5. Wildcard Expansion
   1. Expand wildcards in the path
   2. Expand wildcards in the file specification
6. Mode specific work
   1. Make directory if mode is 'makedir'
   2. Make parent directory if mode is 'makeparentdir'
   3. Check that path exists if mode is 'check'
7. Done!

So the logical precedence is as follows:

syntax for this list is -option1- disables -option2-

type > suffix
suffix > makedir #I'm not sure about this one. What if somebody really wants to make a directory called run1.nii?
wildcard_path > makeparentdir
wildcard_file > makedir

and I think that's it.

[dankessler]

Alright, just added a billion tiny commits that @mangstad can use for testing. Unfortunately I have not had time to test yet...but all I did was move stuff around so hopefully nothing got too broken. If I don't hear back, I'll test tomorrow AM and merge the pull if nobody complains.

[mangstad]

I'm assuming you're going to merge this pull request tomorrow. I'd like to then merge the master to my local repository, and do a rebase on som_batch so that I can use the GenPath command. Would the following be correct:

git merge universe
git checkout som_batch
git rebase develop

That should then give me access to the Genpath stuff right (assuming I execute it after the pull request has resolved)?

[dankessler]

That's a good idea, but the semantics for what you're doing is a little different than how you laid it out.

You'd presumably do the follow

1. fetch all branches from universe (there's only one, called develop)
2. Update your local develop branch to be up to date with universe's develop
3. Rebase som_batch onto your local (now up-to-date develop branch)

commands for this are

git fetch universe
git checkout develop
git merge universe/develop
git checkout som_batch
git rebase develop

[dankessler]

Also yes, this should get pulled today. Have you had a chance to test the final commit @mangstad ?

[dankessler]

Alright, I think it's time to go live with this. I've done a bunch more testing and everything seems fine to me. Unfortunately, I haven't tested every combination of options and wildcard locations (I multiplied out and it's like 800+ possible configurations), but I think it's ready to go so I'm going to merge the pull request.

As we roll out additional features we might find bugs or want to change the usage for this, and that's fine, but I think this is good enough to roll onto the develop branch.