Merged
Conversation
…uring development (will eventually again become a single dhollander algo though, if dhollander_new performs to expectations)
Previously, executable scripts would call a set of functions within the mrtrix3.app module in the appropriate order in order to initialise the consistent command-line interface, parsing, etc.. A copy-and-paste block of code at the top of each executable file would ensure that this module was present within the Python path. This change makes the executable interface more similar to that of the C++ files within the cmd/ folder of MRtrix3. Two functions must be defined within the executable file: 'usage()' and 'execute()'. Then, at the bottom of the executable file, the copy-and-paste code block for locating and importing the mrtrix3.app module appears, but also calls the app.execute() function. This function defined within the app module sets up the interface, and uses the Python inspect module to locate and run the two functions defined within the executable file as appropriate. A major advantage of this approach is that it allows Python exceptions to be thrown by library functions (instead of calling the deprecated app.error() function), which may optionally be caught by the calling code in various circumstances. Standard Python exceptions can also be caught by the app module, and displayed in a manner consistent with the script interface. Additionally, when commands executed via the mrtrix3.run module fail, a dedicated exception class is thrown, which allows both the catching of failed commands in the executing code where permissible, and moves all of the code responsible for handling such command failures to the app module (previously the run module would write a text file, which the app module would then load). A couple of generic capabilities (e.g. reading the MRtrix3 config file(s)) have been moved to __init__.py, such that they are more easily accessible when using the library but not exploiting the app module interface. Various changes to the mrtrix3.run module have been made to support multi-threaded use of the run.command() function. This includes use of tempfile.mkstemp() rather than tempfile.TemporaryFile(), as the latter was found to not be thread-safe in Python3 on Windows. The run.command() function additionally now supports list input, where individual entries may themselves be lists (allowing e.g. concatenation of multiple file paths into a command call, but the terminal display will show only the common prefix/suffix of the list rather than the entire contents). run.command() now additionally supports '||' and '&&' operators.
- dwibiascorrect: Fix cmdline function call. - population_template: mrtrix3.app module needs to be loaded earlier for root-level functions. - generate_user_docs.sh: Fix grep call based on new script syntax. - 5ttgen gif: Add missing import call.
This script replaces average_response, performing the same function but with an executable name more consistent with the rest of MRtrix3. It utilises the MRtrix3 Python scripting library, and therefore appears in the auto-generated documentation. Additionally, while average_response simply averaged response function coefficients directly (meaning that subjects with greater image intensity would have a greater effect on the group average response function shape), responsemean (by default) takes into account any global differences in scaling between subjects when generating the mean. The former behaviour can however be accessed using the -legacy command-line option.
This commit contains a re-implementation of the 'foreach' script using the MRtrix3 Python scripting library, as opposed to the original implementation which is simply a Bash script. This means that the script will be included in the auto-generated documentation. It also means that the terminal output from multiple running commands can be better controlled. This does however involve a change to the user interface of this command: Rather than placing a colon character as a 'no-op' to separate the 'foreach' invocation from the command to be executed multiple times, this new version instead expects the final positional command-line argument input to be a string that contains the command to be executed (including any substitution flags). The documentation has been updated to reflect this.
Python modules 'file' and 'path' have been merged into a single module 'fsys' (filesystem). This in particular prevents overriding of the Python built-in 'file', which was very much not ideal.
Contributor
|
What a delight to wake up to this! 😄 🎉 |
Member
Author
I made the same evolution myself. 👍 |
Member
Author
I hadn't realised that you were such a big fan of all of the work I've done here. |
This was referenced Apr 24, 2019
Merged
Lestropie
added a commit
that referenced
this pull request
May 2, 2019
Residual issue with changes in #1449 that only arises when the CUDA version of eddy does not complete successfully.
This was referenced Aug 9, 2019
This was referenced Sep 25, 2019
Lestropie
added a commit
that referenced
this pull request
Sep 26, 2019
Python3 on TravisCI have introduced new tests with pylint 2.4.0. This change should permit testing on master to pass: - Use sys.exit() rather than exit(). - Remove some unnecessary comprehensions in dwipreproc. - Tweak run_pylint script to not print erroneous warnings due to cache directories or empty directories left over from checking out other branches. - Ignore import-outside-toplevel; given the overhaul of the Python API in #1449 conformance with this recommendation will instead be performed on the dev branch.
Lestropie
added a commit
that referenced
this pull request
Sep 26, 2019
Mirrors f9d1d54, which was applied to master; doing changes independently due to major Python API alterations in #1449. - Remove unnecessary comprehensions in dwifslpreproc. - Move all import statements to outer block at top of file. - build: Use sys.exit(); remove unnecessary comprehension. Python signal handler: Do not use sys.exit() (recommended that signal handlers use os._exit() instead). - run_pylint: Prevent error messages in case of empty directories in lib/mrtrix3/.
Lestropie
added a commit
that referenced
this pull request
Jan 30, 2020
Command return value integer is no longer a part of the CommandReturn named tuple, as since #1449 any command returning a non-zero value will instead raise an MRtrixCmdError.
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mostly addressing #1400, but a few other things in there too; it was kind of a long steady accumulation of various things, I've tried to chop it up into a few commits as best I could.
They're significant library changes, and I don't like making changes that break compatibility with any stand-alone scripts out there, but I think there's strong justification for making the changes. After all, how many times have we modified the C++ library code in getting to the point we're at now?
I'll try to enumerate everything of interest here for comments. Not going to be merging this PR quickly, I should probably be implementing more tests before accepting such large changes; but I wanted to open the proposed changes up for discussion.
(Also I need to get this stuff out of my head so I can focus on more pertinent demands...)
Completely changed how executable script files invoke the core application library.
Previously there was a set of compulsory library functions that needed to be executed in the correct order, e.g.
app.initialise(),app.parse(),app.complete(). Now, one defines two functions:usage()andexecute()(couldn't haveusage()andrun()like in C++, since the latter would clash with therunmodule). After definition of these functions,app.execute()is invoked.app.execute()then sets everything up, using theinspectmodule to find those two defined functions and run them when appropriate.The biggest advantage of this is that it enables the use of exceptions. Instead of calling
app.error(), library functions will throw a custom MRtrix3 exception, which can either be caught by the script programmer, or result in similar error message & cleanup behaviour asapp.error(). This substantially improves the capability for scripts to catch when things don't quite work as intended and adjust appropriately.run.command()gets its own exception, which includes all information about the command and thestdout/stderrcontents. This means that the error messaging and cleanup can occur entirely withinapp.execute()when the exception is called; as opposed to the current solution, which is partially managed byrun.command()writing a text file thatapp.complete()then reads, which is kinda clunky.Moved some basic capabilities, e.g. listing available MRtrix3 binaries / importing the config file(s), to
__init.py__. Anyone using the library functions but not invoking theappmodule in its entirety should still be able to access this information.Changes to
run.command()to support multi-threaded usage.Note that this required the use of
tempfile.mkstemp()rather thantempfile.TemporaryFile()due to an apparent race condition in the latter on Python3 on Windows (not present on Windows Python2; not yet tested on Linux). This is despite the Python documentation suggesting thatTemporaryFile()usesmkstemp()... The side-effect of this is that thestdout/stderrcontents of executed commands are written to files in/tmp/and deleted after they are read; there could potentially be issues with users not having write access to that location and/or accumulation of stale files due to cleanup issues.Porting of
foreachto Python. In addition to appearing in the online documentation, this also means that the terminal will present a simple progress bar showing the execution of however many instances of the command are run, and at completion only invocations that failed, along with theirstdout/stderrcontents, will be printed. Note that the command-line usage is fractionally different to the current bash script.New script
responsemean, which has substantial changes from the basisaverage_response, but I think the name is more consistent with the rest of MRtrix3. In addition to making use of the libraries (and hence appearing in the online documentation), having better error messages, and being a little more Pythonic in places, the (default) averaging process is different, as proposed in average_response: De-mean before averaging #1096.Merge
fileandpathmodules intofsys. Really didn't like having to usepylintdirectives to hide the fact that a Python built-in was being over-ridden.Much nicer terminal feedback when running
population_template, as you don't get humongousrun.command()strings when running e.g.mrmathacross all subjects (implemented the solution proposed in this comment in scripts: allow list input to run.command #928).Partially addresses #692.
Hoping that #927 can be closed, but point 3 may raise other issues regarding the solution.
Closes #928.
Closes #1096.
Closes #1275.
Closes #1338 (script author can simply catch the exception along with
stdout/stderrcontents).Closes #1377 (command returncode is returned in exception raised by
run.command()).Closes #1400.