Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interface name changes #1211

Merged
merged 90 commits into from Dec 18, 2017
Merged

Interface name changes #1211

merged 90 commits into from Dec 18, 2017

Conversation

thijsdhollander
Copy link
Contributor

Starting to implement some name changes on the TODO list (#1147). I started with the least controversial one (-stride --> -strides). It would be good to get some of this done if we prefer to do a dev to master release soon, for the sake of getting #1206 out.

I marked this as "help / test wanted" because some of this requires widespread scattered changes, in code as well as scripts and documentation. It's very easy to overlook some bits here... so if anyone finds time: always welcome to help out, and even just check that nothing gets forgotten.

Lestropie and others added 30 commits August 22, 2017 16:35
- Re-order and modify page on fixels and dixels to emphasize that 'fixel' is the dominant term, and 'dixel' may not be relevant for many readers.
- Couple of spelling errors fixed.
- Add instructions on how to deal with 'too many open files' error.
This page is not "Crashes, errors and performance issues".
In addition, some entries that were duplicated between two pages were removed, and some pages from FAQs have been moved into this page.
I'm getting questions about this all the time, and the most convenient and accurate solution has always been to use this algorithm. If this for some very specific reason didn't work well, msmt_5tt didn't work either; and either the -fa parameter of dhollander could be tweaked for a good solution, or, worst case, manual selection of responses was the last resort.  Offering a quick, convenient and accurate solution has always been my intention here, with the aim of multi-tissue csd to be more accessible, and hence to encourage its adoption. It's also good for the perception of msmt-csd that it produces a good quality output; which does depend on good quality responses, obtained without hassle.
This section about setting the CPU architecture was included in the
standalone installer instructions due to the fact that we used to run
with -march=native - this was likely to cause issues for this type of
installation specifically. Now that the default is up to the compiler,
there is no longer any reason for this section to be where it is.
More to follow, this is a work in progress at this stage.  Just committing now to check the readthedocs output for layout.
- tck2connectome: Add references.
- FAQ: Add descriptions of some common issues encountered, including: tcknormalise requiring warpcorrect to be applied beforehand; 5ttgen fsl failing at the FIRST segmentation step.
…ect to the Tournier et al. 2013 publication, differences with respect to 'tax' algorithm
@thijsdhollander
Copy link
Contributor Author

Bonus: via this, the latest master changes will also end up on dev. 🎉

@bjeurissen
Copy link
Member

@dchristiaens: I also have dwimotioncorrect in my development code. I really hope this does not mean we have been working on the same stuff again... We should talk!

@thijsdhollander
Copy link
Contributor Author

thijsdhollander commented Dec 18, 2017

@Lestropie , I suppose you may understand this better than I do; see these tests failing: https://travis-ci.org/MRtrix3/mrtrix3/jobs/317859241

From my end, it seems more (pylint) tests started failing since I've merged in dev just today (which included the commits from 2 pull requests you've merged into dev a couple of days ago).

EDIT: I've probably got it figured out...

@thijsdhollander
Copy link
Contributor Author

FYI, in preparation for this merging to dev, I've created a dev branch on the test data repo as well. We'll need this, now and potentially in the future, in case test data gets changed/removed (anything but just incrementally added) in context of developments on dev or feature branches.

@thijsdhollander
Copy link
Contributor Author

Alright, there we go. Just to stress the above again: there's now also a dev branch on the test data repo, which at the moment includes the changes that have been made here (due to renaming of a few commands). Since there's 2 other outstanding branches on the test repo, they'll need to have dev merged in them for their respective features to eventually go in dev on the main repo.

@thijsdhollander thijsdhollander merged commit d9cb4f3 into dev Dec 18, 2017
@thijsdhollander thijsdhollander deleted the interface_name_changes branch December 18, 2017 06:15
@thijsdhollander
Copy link
Contributor Author

For anyone who wishes to continue discussing anything interface (naming) related, please resort to #1147. The issue is left open for that purpose. Should it lead to anything, happily create another branch and pull request.

Lestropie added a commit that referenced this pull request Dec 20, 2017
Also solves issues of inconsistent return statements reported by newer versions of pylint, reverting 8bb9eb9 merged in #1211.
Lestropie added a commit that referenced this pull request Dec 20, 2017
Rather than disabling this Pylint warning, instead use more Pythoninc approaches where warnings were issued.
Reverts b54c55a committed to dev in #1211.
Lestropie added a commit that referenced this pull request Jan 30, 2018
mrinfo's command-line option -shellvalues was changed to -shell_bvalues in 29dbf02, merged to dev branch in #1211, but a couple of usages of this option in MRtrix3 scripts were not updated.
Lestropie added a commit that referenced this pull request Mar 15, 2018
Was not updated with other changes in 2258b08 that were merged to dev branch in #1211.
Lestropie added a commit that referenced this pull request Mar 15, 2018
Was not updated with other changes in 2258b08 (-stride -> -strides) that were merged to dev branch in #1211.
@Lestropie
Copy link
Member

Late to the party, but:

Should mrmesh be voxel2mesh? It strictly only works for 3D data.

@jdtournier
Copy link
Member

Should mrmesh be voxel2mesh? It strictly only works for 3D data.

Up to you. I think the mr prefix makes it clear that it expects an image as input - even if that would effectively only be a 3D image, but I don't really see that as a problem. The only other command that starts with voxel is voxel2fixel, which is warranted in this case to highlight the conversion from voxel-wise to fixel-wise data. I guess a similar argument could be made for converting between voxel-wise to surface mesh data? But I don't feel particularly strongly either way...

Lestropie added a commit that referenced this pull request Apr 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants