Conversation
In adding features to the Python scripts in order to make them compatible with packaging via PyInstaller, much of the library has been shuffled around. Most noticeable difference is that individual functions are no longer defined in their own library file; instead, some semblence of function grouping has been applied. This means that library function calls will typically have 'lib.' and a module name before the function name. For PyInstaller, differences are in scripts/lib/algorithm.py and scripts/algorithm/package.py; getting PyInstaller to pull in an appropriately read from the algorithm files in scripts/src/ is the trickiest part of the process.
Also had to rename lib.message.print to lib.message.console, since PyInstaller didn't like the former.
- Python library files have been re-arranged: There is no longer one function per file, instead there is some degree of grouping of functionalities into modules. - Various conformations to the FHS restructure: - Scripts now reside in the bin/ directory. - Script libraries now reside in lib/mrtrix3/. - Additional data used by scripts now reside in share/mrtrix3/, in sub-directories according to the script using the data. - The example connectome lookup table files have been moved to share/mrtrix3/labelconvert.
|
Just had a look at this, and merged with
I reckon that would clean things up further... I also removed one import line that caused an error when trying to use All good otherwise! |
|
With regards to merging of files / 'modules', I'm a bit conflicted as to how far to go. There's definitely some sense to Daan's suggestion in thread #896 (although I'd be tempted to just put it all together into a single MRtrix module if we were to go that far), and some of Donald's suggestions are definitely logical, but I'm not sure whether or not to adopt all of them. One thing I found when doing these changes is that a lot of function names could be shortened because a part of the context / purpose of the function became encapsulated within the module name; if you merge too many things, you lose this sense of 'namespace' / hierarchical functionality. I suppose I'll just have a go at some point and see where I end up. |
There's no need to get too worried about this, the scripts work fine as-is, the changes @dchristiaens & I have suggested are purely cosmetic - just to try to make the scripts a little bit more 'pythonic'. But given that they're not really designed for 'external' use (even though there's nothing stopping anyone from using them as such), and not very 'pythonic' anyway given the way we locate the modules and the way the modules locate the binaries, I really don't think this is anything we need to worry about. My suggestion would reduce the number of modules to 6, by my count. So the functionality still remains pretty spread out. I think your logic of shortening the function names using their module prefix is very much the right way to go. So take whichever bits of our suggestions you think make sense and ignore the rest, no-one will be offended... 😉 |
jdtournier
left a comment
There was a problem hiding this comment.
Looks clean, happy with that.
|
Looks good to me too. |
|
I'm good with the restructuring of the "modules", but I'm just slightly concerned (or probably just confused 😕) about the But more generally, as I was trying to wrap my mind around the purpose of these additions, I stumbled upon this... which had me slightly more worried about the (different) behaviours this may lead to... what do you think? |
Unnecessary safety measure put in place because of a different problem. I'll be going back and removing them all again, especially since it's warned against. I've gotten caught a couple of times thinking that not all versions of |
|
@draffelt or @maxpietsch: Can we do something about these? I'd rather have such functionality within |
|
Correct me if I am wrong but I don't think there is any trickery going on, they are more convenience functions. Do you want to replace them with something else or just move them over to path.py? Symlinks were an issue using More important are the runFunction calls like this to ensure that the |
Related to #864, #896.
So this ended up being a bit of a mishmash: I needed to modify the work I had done in the
package_scriptsbranch, both to remove the actualpackage_scriptsscript & related functionality in order to still use the other changes I'd made in that branch, but also to do the required modifications to bring the script library in line with the FHS restructure. It looked like it was going to be more effort to split up the two jobs, so I've done both in the one go. Diff is a bit evil (a lot of files are flagged as new & delete rather than a move), so you might have a more fun time just looking at the branch itself.Open to comments not only on the FHS restructure, but also the 'module-ing' of the Python code.
Only thing left in the
scripts/directory isgenerate_mrtrix_completion.py: @rtabbara This probably needs to be modified to bring it up to date. It should perhaps also go into the root directory now?Edit: Got
dwi2responseworking, but all of the scripts need to be re-tested when I'm not on a Windows machine.