edited documentation 5ttgen hsvs -white_stem#2330
edited documentation 5ttgen hsvs -white_stem#2330nicdc wants to merge 21 commits intoMRtrix3:masterfrom
Conversation
added a line to the documentation for 5ttgen hsvs to explain that using the -white_stem option would lead to rejection of streamlines terminating in the brainstem region
Contribution by Xue Li on MRTrix Community forum
added copyright
added copyright
Recreated in Unix
…therwise run that step repeatedly. Also takes away the FSL dependency in labelsgmfix for Windows users.
update before pushing changes
…that users might not usually perform
|
Detailed changes:
I ran this on my own data and it works as expected. There is still room for optimization - was not sure how to include wildcards in the fsl.find_image call to handle data with different prefixes etc. Would be happy to have some feedback (@jdtournier @Lestropie ) Cheers, |
|
Hi @nicdc, Sorry for taking forever to comment on this; I'm way behind on a lot of things right now... It looks like the scope of the contents of the PR have increased a bit since it was first listed, which means the title no longer describes the scope of changes. You or I can edit the PR title accordingly, or it can be split into multiple distinct PRs, each with its own distinct purpose. The latter tends to be better for tracking from a software engineering perspective, though it's hard for me to demand such if I'm not responding to them promptly... 🤦 The addition of the parcellation LUTs is however sufficiently distinct from I'll add other comments to the relevant code snippets. Cheers |
| - **-premasked** Indicate that brain masking has already been applied to the input image | ||
|
|
||
| - **-first_dir /path/to/first/dir** use output of FSL FIRST if it has been previously run on input T1-weighted image, in the SAME SPACE as input T1 | ||
| - **-first_dir /path/to/first/dir** use output of FSL FIRST if it has been previously run on input T1-weighted image, in the SAME SPACE as input T1. Filename prefix should be first- |
There was a problem hiding this comment.
The main reason why one would be using a capability such as this is if it is intended for these data to be used in multiple subsequent pipelines; demanding a specific naming convention may undermine such. What would be preferable would be:
- Query the contents of the filesystem directory;
- Search for prefix strings for which all of the requisite file names are present;
- Proceed if the number of such unique prefixes is 1.
| run.command('cp ' + path.from_user(vtk_in_path) + ' ' + path.to_scratch(vtk_in_path)) | ||
| run.command('cp -r ' + path.from_user('first.logs') + ' ' + path.to_scratch('first.logs')) |
There was a problem hiding this comment.
Using the shutil module (via run.function()) is preferable to run.command(cp ...).
| options.add_argument('-premasked', action='store_true', help='Indicate that brain masking has already been applied to the input image') | ||
| options.add_argument('-first_dir', metavar='/path/to/first/dir', help='use output of FSL FIRST if it has been previously run on input T1-weighted image, in the SAME SPACE as input T1') | ||
| options.add_argument('-first_dir', metavar='/path/to/first/dir', help='use output of FSL FIRST if it has been previously run on input T1-weighted image, in the SAME SPACE as input T1. Filename prefix should be first-') | ||
| options.add_argument('-fast_dir', metavar='/path/to/fast/dir', help='use output of FSL FAST if it has been previously run on input T1-weighted image, in the SAME SPACE as input T1. Filename prefix should be T1_BET') |
There was a problem hiding this comment.
Same as prior comment RE: file prefix
| if not os.path.isdir(os.path.abspath(app.ARGS.fast_dir)): | ||
| raise MRtrixError('FAST directory cannot be found, please check path') | ||
| fast_output_prefix = fast_t1_input.split('.')[0] | ||
| fast_csf_input = fast_output_prefix + 'pve_0.nii.gz' |
There was a problem hiding this comment.
Is there an underscore missing here?
|
In addition, we're trying to be a bit more comprehensive with testing; execution times can be prohibitive for testing of Python scripts, but we do nevertheless try to implement tests for Python scripts that can be executed manually prior to tag releases. The whole |
For files with different prefixes, it'd just be a matter of running the function once for each prefix. The function is only there because there are specific instances in which relying on an e.g. You can also be clever in instances like this with list comprehensions; e.g.: |
|
I might add that some of these changes (documentation updates) can go straight to Apologies if this is a bit of a pain, but we're trying to stick to a predictable release pattern - something we've not been very good at in the past, which has at times caused some frustration with users. |
|
Added to |
Manual application of changes in #2330 by: Robert E. Smith <[email protected]>
|
Closing as content has been separated out into topics with dedicated PRs. |
Manual application of changes in #2330 by: Robert E. Smith <[email protected]>
3 participants
Please enter a concise description of the changes proposed in the pull request,
along with any relevant links to external discussions.
If your pull request involves fixing a bug, then the relevant code
should be in a branch that is a derivative of
master, and you shouldselect
masteras the target branch in the pull request; for all otherpull requests, the relevant code should be in a branch that is a
derivative of
dev, and you should selectdevas the target branchof the pull request (assuming no other more appropriate branch exists).
If your proposed changes address an existing Issue
listed on GitHub, please add a reference to that Issue (a "
#"character followed by the issue number).
If your proposed changes are not yet ready to be merged, but you are
creating a pull request in order to initiate communications on such,
please flag it as a draft pull request.
You may optionally add
@mentionsof a person or team appropriatefor reviewing proposed changes.
@Lestropie