Skip to content

tckgen: change default angles#1630

Closed
jdtournier wants to merge 3 commits intodevfrom
tckgen_det_angle_defaults
Closed

tckgen: change default angles#1630
jdtournier wants to merge 3 commits intodevfrom
tckgen_det_angle_defaults

Conversation

@jdtournier
Copy link
Copy Markdown
Member

first attempt at sorting out #930

@Lestropie
Copy link
Copy Markdown
Member

To what extent was this value trialled? I was expecting to have to generate another parameter matrix for each algorithm.

@jdtournier
Copy link
Copy Markdown
Member Author

jdtournier commented Jun 26, 2019

Not trialled at all. All 100% gut feeling...

That said, I don't expect this parameter to have as much influence as the cutoff did. I also don't expect everyone will agree with any default we come up with. For deterministic algorithms, 60° 'feels' right: 90° is clearly too liberal, and likely to allow too many U turns, and something like 30° will probably be too restrictive. I reckon somewhere between 45°and 70° is likely to work ok in most cases, for most deterministic algorithms - results looked sensible enough for all algorithms when I tried them out, in any case...

@jdtournier jdtournier added this to the MRtrix3 3.0 release milestone Jun 26, 2019
@jdtournier jdtournier self-assigned this Jun 26, 2019
@jdtournier jdtournier requested a review from a team June 26, 2019 19:16
@Lestropie
Copy link
Copy Markdown
Member

Really not as informative as I'd hoped...


FACT:

FACT_compiled_small
Full resolution


SD_STREAM:

SDSTREAM_compiled_small
Full resolution


Tensor_det:

Tensordet_compiled_small
Full resolution

@thijsdhollander
Copy link
Copy Markdown
Contributor

For deterministic algorithms, 60° 'feels' right: 90° is clearly too liberal, and likely to allow too many U turns, and something like 30° will probably be too restrictive. I reckon somewhere between 45°and 70° is likely to work ok in most cases, for most deterministic algorithms - results looked sensible enough for all algorithms when I tried them out, in any case...

I have the same experience in practice. I think 60° is a good default.

@jdtournier
Copy link
Copy Markdown
Member Author

Just looking into this issue again, I note we haven't come to a conclusion on this one, but also that there are merge conflicts here related to the changes in c18e368 (part of #1507), some of which influence the default step size in the deterministic algorithms we're discussing here - but the basic mechanism to set the step size is still based on a consideration of allowed radius of curvature, which as discussed previously, doesn't really work in a deterministic context (doesn't really work in a probabilistic context either, to be fair - which is why the option to set this is now called -angle rather than -curvature).

How do we want to handle this? I'm confident that a constant angle is best for the deterministic algorithms - irrespective of step size - as has been highlighted on the forum (we can argue about the 60° default, but I reckon it's as good as any). Is everyone happy to change to that, as per this pull request? In which case I'll need to figure out how to sort out the merge conflicts without screwing things up...

I'm also concerned about the change in default angle introduced in that same commit (c18e368) going from 90°×step/vox (i.e. 90° after having travelled a voxel's worth of distance) to 2asin(step/2vox) (i.e. a radius of curvature of a voxel). By my reckoning, for a step size = ½vox (as used in iFOD2), this gives a default max angle of 29°, compared to the current default of 45° - quite a big difference, and at odds with the statement made in this post on #1507:

This does however mean a change to default tracking behaviour for some algorithms (not iFOD2 though).

Am I missing something? I don't think there's any reason to change the defaults on either iFOD1 or iFOD2 as things stand, but it looks to me like these are now different between master and dev, and I don't see a justification for changing these, other than the elegance of the radius of curvature approach. I see the need to compute the radius of curvature given the complexities of computing the streamline length correctly though - but I don't see that this therefore means we have to set the defaults using that approach?

@Lestropie
Copy link
Copy Markdown
Member

the basic mechanism to set the step size is still based on a consideration of allowed radius of curvature, which as discussed previously, doesn't really work in a deterministic context

I'm confident that a constant angle is best for the deterministic algorithms - irrespective of step size

I don't think it's actually fundamentally a matter of probabilistic vs deterministic. Obviously the way the permissible angle is used is quite different between the two. But I think the more crucial dimension is interpolation & the smoothness of the underlying model under such. E.g. in the extreme case where nearest-neighbour interpolation is used (fact), forcing a small angle per small step won't work, as the direction changes will always be abrupt regardless of step size, so a fixed angle makes sense. For something like tensor_det, if the eigenvector orientation rotates smoothly as the sub-voxel position is varied, then it does make sense for the permissible rotation angle to depend on the step size. FOD-based interpolation for sd_stream only works up to a certain extent; if the angle change between adjacent voxels is too large then the peak orientation won't slerp between the two. But overall, I'd say that a fixed angle is preferable for these algorithms because interpolation doesn't yield smoothly varying orientations for small step sizes; the fact that these algorithms are non-stochastic is actually slightly secondary.

(we can argue about the 60° default, but I reckon it's as good as any)

Can't make a good argument for anything else.

Is everyone happy to change to that, as per this pull request?

Yea I think just do it.

By my reckoning, for a step size = ½vox (as used in iFOD2), this gives a default max angle of 29°, compared to the current default of 45° - quite a big difference, and at odds with the statement made in this post on #1507:

This does however mean a change to default tracking behaviour for some algorithms (not iFOD2 though).

Am I missing something?

The point of that comment is that the empirical behaviour of iFOD2 should have been unaffected by those changes. Taking a quick look at the code though it's possible that that's not the case. I'll need to have a poke around.

For iFOD1 it makes sense to me to set the angle that yields a one voxel radius of curvature; the "90° after having travelled a voxel's worth of distance" feels like a hack because of having changed from -curvature to -angle; the radius of curvature is the intention behind the derivation, but the derivation is not precise. For the smaller default step size of iFOD1 the difference between the two should be considerably less (too long ago to remember the numbers).

@jdtournier
Copy link
Copy Markdown
Member Author

Right, good that we're converging on a fixed angle, at least for deterministic approaches. I'll try to merge this into dev, see how we go.

On the probabilistic side, I'm actually starting to question the whole radius of curvature approach. It's appealing, but I'm not sure it works in practice. Here's where my brain is at currently:

iFOD1

For iFOD1, the radius of curvature argument yields a default angle of approx. 9° on master (90°×step/vox, with step defaulting to 0.1vox). This used to be 11.5° in MRtrix 0.2 (step=0.2mm, radius of curvature=1mm). On dev, I reckon we'd end up with 6° unless I'm not reading the code right. The problem is that as the step size reduces, the whole thing falls apart: a step size of 0.01vox would produce a default angle of ~1°, which prevents the algorithm from curving effectively. This is because over a 1-2° cone, the FOD is effectively uniform, so we're sampling from a close to uniform distribution, with the average orientation being straight ahead. On the other hand, for large step sizes, the default angle becomes too large, and the results get pretty messy. Here's a screenshot to illustrate (left: default step size ~0.23mm; middle: step=0.01mm; right: step size=1mm).

ifod1_default

So the default only really works in combination with the default step size. If we veer too far from that default, the approach is not adequate.

On the other hand, if we set a fixed default angle, not tied to the step size, the results come out more consistent - as long as the default angle is sensible. The following screenshots are equivalent to the above, but with -angle 15 (seems to work best on that front):

ifod1_angle_15

Note that the results with small step sizes look a bit 'deterministic'. That's because the repeated sampling over short intervals means the trajectory effectively self-averages. There's jitter if you zoom in (a lot), but you can't see it over the more relevant voxel scale.

iFOD2

It's a similar argument for iFOD2, with a similar outcome. The screenshot below is left: iFOD2 defaults (step size=½vox = 1.2mm here, angle=45°) on the left; middle: step size=0.1mm (angle=~4°); right: step size=5mm (angle=~195° 😜).

ifod2_default

The one below is the equivalent set of screenshots, with the angle fixed to 45°:

ifod2_fixed_45

Clearly these are all different, since the step size does have an effect - but I reckon they're more self-consistent than those based on the step size...

Cutting a long story short

So basically, I'm leaning in favour of a fixed angle, with no dependence on step size, for all algorithms. The default would obviously be algorithm-dependent, but I have a feeling this would otherwise lead to more consistent results if the step size is varied - in contrast to the radius of curvature argument (or anything that depends on step size), which somewhat counter-intuitively leads to quite different outcomes depending on the step size.

The added advantage of such an approach is that it's a simpler default to provide to users - no need to work it out based on the parameters, it is what it is. And if users decide to change the step size, they'll need to consider whether the angle is adequate regardless of how we set the default - but using a fixed default makes less of difference that using a step-size dependent one.

As to what we do about this, I have a feeling this will have to be discussed and assessed more carefully when we have a bit more time on our hands - along with the default cutoff if we decide to use @thijsdhollander's new dwi2response estimator by default (#1689). Both of these have a non-negligible impact on what happens in tckgen, so it'll be worth getting it right.

Thoughts welcome, but in the meantime, we might just have to park these discussions until the workshop is out of the way...

@Lestropie Lestropie changed the title tckgen: make all deterministic algorithms default to 60° angle tckgen: change default angles Oct 21, 2019
@Lestropie
Copy link
Copy Markdown
Member

Changed PR name to reflect most recent discussions. Most likely all algorithms are going to be changed to use a fixed maximal turning angle, rather than being derived based on the step size.

@thijsdhollander
Copy link
Copy Markdown
Contributor

As to what we do about this, I have a feeling this will have to be discussed and assessed more carefully when we have a bit more time on our hands - along with the default cutoff if we decide to use @thijsdhollander's new dwi2response estimator by default (#1689). Both of these have a non-negligible impact on what happens in tckgen, so it'll be worth getting it right.

Just my 2 cents; thinking about how all this interacts together, I reckon if it's too "chaotic" to consider everything at once (cutoff & angles), I think the safest or simplest way is to first get the cutoff right (while of course already taking into account changes to everything having to do with FOD amplitudes themselves; i.e. recent changes in evaluation of such and potential changes due to different scale of response functions, due to different dwi2response estimator), and then consider the angles given whatever ends up being the new default cutoff. In general, you could do it either way; however, the cutoff can be considered "somewhat in isolation" at least, because you can evaluate it even without tractography: it's just about which lobes (fixels) are desirable versus those that are false positives. The angle, on the other hand, is linked uniquely to tractography, and then the dependency on the cutoff and its effects unavoidably come into play. For probabilistic algorithms, this probably matters the most, as the cutoff not only decides whether a lobe "is in or not", but also the size/area of the angular domain that is considered.

That said, the arguments brought up so far here about the angle do make sense so far in their own right. So alternatively, you could just set it (the angle) to some reasonable value based on that; and then just/only consider the cutoff, and just see if what it all ends up being provides a decent output. In any case, it remains a heuristic thing, so you could argue the defaults only have to be somewhat empirically reasonable.

But anyway, whatever the approach (to figure out all defaults), its probably not a bad thing to just choose one such approach first and then go for it; it's going to be very messy otherwise, I reckon.

Resolve divergence between #1630 and #1507. Use a single function Tractography::Shared::set_step_and_angle() that is responsible for setting up both the step size and maximum deviation angle per step. Remove calculation of maximum angle based on radius of curvature, in favour of a fixed angle for each algorithm. Use separate member variables for maximum angles and related quantities between first-order and higher-order integration as was introduced in #1507.

Conflicts:
	src/dwi/tractography/algorithms/fact.h
	src/dwi/tractography/algorithms/iFOD1.h
	src/dwi/tractography/algorithms/iFOD2.h
	src/dwi/tractography/algorithms/nulldist.h
	src/dwi/tractography/algorithms/sd_stream.h
	src/dwi/tractography/algorithms/seedtest.h
	src/dwi/tractography/algorithms/tensor_det.h
	src/dwi/tractography/tracking/shared.cpp
	src/dwi/tractography/tracking/shared.h
	src/dwi/tractography/tracking/tractography.cpp
Missed unused variable in the process of generating merge commit bc68b7d.
@Lestropie
Copy link
Copy Markdown
Member

Superseded by #1833.

@Lestropie Lestropie closed this Jan 8, 2020
@Lestropie Lestropie deleted the tckgen_det_angle_defaults branch January 11, 2020 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants