mrgrid and mrtransform: check spatial stride order#2460
mrgrid and mrtransform: check spatial stride order#2460maxpietsch wants to merge 3832 commits intodevfrom
Conversation
The FSL eddy commands sometimes contain a small block of binary data at the end of their outputs. If, when attempting to write those data to a text file in the scratchh directory, an encoding operation occurs, such data may yield a UnicodeEncodeError. This commit changes the output text files to be opened in binary mode, so such an encoding should no longer be applied.
dwifslpreproc: Handle non-ascii data in eddy error outputs
- On EddyQC failure, write text file containing stdout / stderr contents in binary mode, just as is done for eddy (#2313). - For EddyQC exports, deal with prospect of whitespace in output directory path.
Container recipes
Echoes 0f3cc37, but isolated changes to tests only for integration into 3.0.3.
This environment variable modulates the terminal verbosity of commands in the same fashion as the -quiet / -info / -debug command-line options. The contents of this environment variable are utilised by the Python API to affect the verbosity of any MRtrix3 commands that it invokes, rather than explicitly adding the -info flag to the execution string when relevant. The MRTRIX_QUIET environment variable remains, and supersedes MRTRIX_LOGLEVEL. Additional fixes include Python commands now correctly honouring MRTRIX_QUIET, and progress bars in Python scripts no longer being erroneously displayed when the log level is zero. Fixes #2268.
Address new refactoring message "consider-using-with".
subprocess.Popen only supports context management as of Python 3.2. As such, MRtrix 3.0.3 requires reversion of some of the changes in c124de1.
dwifslpreproc: Fix eddy text file outputs (again)
New envvar MRTRIX_LOGLEVEL
Additionally includes removing use of subprocess.Popen as a context manager within the mrtrix3.image module. Conflicts: lib/mrtrix3/image.py
DICOM: fix formatting error in dictionary
changed tck2connectome to connectome2tck
Update connectome2tck.cpp
regular merge of master to dev
Remove Eigen helper functions is_finite() and is_nan()
- Don't write debugging images relating to untracked fixels, given that this can be trivially inferred from other data. - Minor changes to image paths and variable names. - Exprt debugging images during tests to ensure successful execution.
Math::Sinc: Fix validity assertions
…handling fixelconnectivity: Normal image handling
tcksift2: -output_debug to fixel directory format
- Clarify code responsible for detection of empty mapping. - Include in test suite verification that use of mismatched lookup table files results in a non-zero command return code.
…_mapping labelconvert: Throw exception on empty mapping
|
I like the idea of better documenting the strides issue, but I'm not sure a full-blown warning is a good idea. I have a feeling that will produce a lot of unnecessary warnings in users' existing pipelines, and raise a lot more questions on the forum. How about, instead of issuing warnings, adding a more explicit paragraph in the command description, making mention of NIfTI specifically and the issue of interoperability with other packages? Even better, add a few example usages, like we do with mrconvert (code, docs), including how to deal with the NIfTI situation in external packages (i.e. the use case that triggered these changes on the forum). That way, there is a documented way to deal with this that users faced with this issue should hopefully come across (maybe with a quick search), and we don't issue potentially unnecessary warnings. Alternatively, I would be OK with issuing the warning if both reference and output images are in NIfTI format... |
|
No strong opinion either way but I think a warning message is justified here, as the explicit operation to regrid to a reference image might be understood in different ways, the most conservative and potentially most common one including matching strides. As the definition of this operation is somewhat ill- or undefined and debugging these issues can take a long time (recent experience....) I think it's fair to use a warning level message in the same spirit as Only triggering warnings for NIfTI gets rid of some unnecessary warnings but |
|
For what it's worth, I think Romain has a good point, fundamentally. On the one hand, yes, the common logic among commands might be that properties of the input are preserved as much as possible, which from that point of view "in principle" applies to the strides. However, on the other hand, one can argue that the template here is an important part of the input: the fact that it happens to be provided via an option is just interface semantics; or a consequence of the fact that the "input" that tells us what/how the final grid should look like can come in a range of ways, of which some are inherently present in the other input, and hence the other possible ways are "optional", which then dictates the interface. But if you forget the interface practicalities, then the scenario where a template is provided means that the template provides important input; i.e. the final grid. The question then becomes whether you regard the strides as a property of "the grid" or not. If a property of the grid, then one can argue the strides of the template should by default end up in the output, just like the grid's resolution, dimensions and transform do. For the latter properties it's of course unavoidable, whereas for the strides it works both ways... in MRtrix. Anyways, all that to say that it can easily be justified either way. That said, I keep running into users that expect the strides to come along from the template; just last week in a group discussion at an event (and these were methods developers). Some like Romain expect this explicitly indeed, knowing the ins and outs of strides very well. Others, who don't know what strides even are, expect this implicitly and don't understand why their outputs don't work in other software; especially when those outputs come directly from Yes, you can solve this with a warning in theory, but for those users who don't know what strides are, it won't give them peace of mind. Even worse, you'll end up having to explain strides to those users that didn't even have a problem to begin with (i.e. when the warning wasn't practically relevant to them). Then only making that warning appear for some file types is a bit like a patch on a patch; i.e. it only gets more complicated, as do the explanations when someone asks for it ("I only get this warning for file type X, and not Y; why?" ==> question isn't even about strides, yet the answer is a long story involving strides). If you do go for a warning, I would argue you better simply always make it appear (regardless of output file type), but maybe make it verbose enough so it says something like "this (warning) is typically only relevant in context of interoperability with other software"; maybe with an optional link to the history of file types, software, and their ways of handling strides... but I think the "accessible" warning message will be the most valuable part in this for most users. |
|
I have to admit, I'm also leaning towards matching the strides of the template by default when the But I am wary of unwittingly turning volume-contiguous data into spatially-contiguous data, simply because the template provided was 3D or a spatially-contiguous 4D image (e.g. a NIfTI). I think it might be enough to grab the ordering of the spatial strides of the template, and apply them to the output, preserving the strides of the non-spatial axes. It'll make no difference for NIfTI outputs anyway (they don't support volume-contiguous storage). And if users do want to preserve the strides of the input, it would be trivial to supply that through the existing Since this would change behaviour a bit, I reckon such a modification would probably best be made on the |
I think that sounds very sensible. There is a justifiable logic here too, if need be: there is no interpolation/resampling along the non-spatial dimensions in these commands, so the non-spatial dimensions can be seen as separate from the (spatial 3D) grid. Put differently, only the spatial grid (which would then include the spatial strides) is pulled from |
Lestropie
force-pushed
the
template_stride_check
branch
from
be262c2 to
8539074
Compare
Lestropie
force-pushed
the
dev
branch
2 times, most recently
from
70031c3 to
6bf4cec
Compare
|
Replaced with #3186. |
11 participants
As raised on the forum, this checks the spatial strides of output and template image and, if no stride option was provided, warns if their order does not match.