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

Fix SH precomputer and update associated thresholds so existing practices and pipelines don't break. #1228

Merged
merged 29 commits into from Apr 25, 2018

Conversation

Projects
None yet
3 participants
@thijsdhollander
Copy link
Member

thijsdhollander commented Jan 3, 2018

Continuing work from #1206, which wasn't completed yet.

Relevant:

2 dot points from this post: #1206 (comment)

This list of dot points: #1206 (comment)

For convenience, here's the latter:

  1. check the actual fix itself or course
  2. check default tckgen FOD cutoff; as indicated in #1206 , I think this should consequently be lowered (significantly)
  3. probably same thing for the default -fmls_peak_value in fod2fixel
  4. in case the previous bullet (3) is indeed a thing, lower the "default" recommendations for this parameter in both (single tissue and multi tissue) FBA pipelines in the userdocs
  5. potentially similar changes to such values in other userdocs pages
  6. others...?

Number (1) is done, the rest is not. We can't deploy this fix without addressing these others, or behaviour will be changed significantly and in an unwanted way, breaking people's pipelines' intention. I will comment further on the thresholds next week.

Just an insignificant commit, otherwise I can't change the base branc…
…h of the pull request to dev, so it seems.

@thijsdhollander thijsdhollander changed the base branch from master to dev Jan 3, 2018

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Jan 3, 2018

The insignificant commit is to be able to set the base branch to dev, which wasn't possible otherwise (GitHub complained) since all existing commits already exist on dev too.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Jan 3, 2018

Finally, note that automatically updating this with dev may bring in the revert, which we don't want here of course. We need to essentially merge dev inhere while keeping "our" version, not dev's. I can't do this from here; I've been doing everything needed for the revert via the GitHub website here, since I'm not at work during my leave.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Jan 8, 2018

Ok, we're set up again, ready to tackle the thresholds.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Jan 8, 2018

⚠️ Ok, first quite unexpected observation... so I've set this up again on purpose so dev reflects the situation before the bug fix, and this branch and pull request the situation after the bug fix, where I was thinking of bringing in those thresholds along "at the same time", so we get to see the impact before and after.

At the moment, the thresholds are untouched (0.1). dev doesn't have the bug fix, this branch does. So I performed an fod2fixel on an FOD image, with all parameters set to defaults. This implies just a straightforward 0.1 amplitude threshold on the FOD, and every "cluster" (in the angular domain) of amplitudes above the threshold becomes a separate fixel. I ran this using fod2foxel from dev and fod2fixel from this branch. I expected to see a difference, as we reasoned before... but there was none. Exactly the same fixels get segmented. I also outputted the -peak (amplitude) image to verify I was definitely using the 2 different versions (before and after fix), and those do look different (as expected). Furthermore, I see fixels with a (wrong) peak amplitude below the 0.1 threshold from the old (dev) version, whereas with the new one, all peak amplitudes from the segmented fixels are correctly above 0.1.

Conclusion being: I suppose only the -peak output computation is using the SH precomputer, and the threshold was already tested against amplitudes that are not using the SH precomputer.

This is great news, and a big relief for all FBA pipeline users... the only change that is needed here ends up being the one unrelated to the SH precomputer fix: just a lower number in the documentation of the FBA pipeline, in line with our experiences here across a broad range of FBA studies. But the behaviour before and after the fix for the FBA pipeline remains the same! So as to fod2fixel, the only scenario that was really affected in practice by the bug is the tournier algorithm, because it's the only one that uses the -peak output.

I'll patch up the FBA documentation with the unrelated change to the threshold advice, the way we've been doing it here for all our studies since several years now.

Only remaining thing on the TODO list (if the TODO is complete of course) is then tckgen. I don't know yet, but I already anticipate a possibility of seeing the same thing there...? I.e., that the thresholds weren't tested against SH precomputer amplitudes? But that the SH precomputer amplitudes were only used for the probabilities in probabilistic tractography? Just guessing at this stage... we'll need to test.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Jan 8, 2018

Ok, minimal adjustment of the docs done in line with experience. I've been wanting to unify both SS and MS FBA pipelines into a single one, again in line with experience. We've got a few better practices for both pipelines around here that can unify both. But I'll leave that to a separate feature branch, as that's indeed independent from this.

Again, what I think only remains now here is to check tckgen.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Jan 8, 2018

A quick test reveals we're not as lucky with tckgen: the threshold (0.1 by default) is effectively evaluated against SH precomputer amplitudes, which are indeed lowered by almost 40% after implementing the bug fix. That's it for the day for me now; feel free to catch up with all of the above. My first impressions with tckgen would motivate me to suggest a change of default threshold, potentially to 0.05, but I haven't done much testing yet.

FOD FMLS: Apply peak amp threshold after peak revision
This means that if the maximal amplitude value sampled for a lobe from the dixel set is just below the threshold, but Newton revision on the sphere yields an amplitude that is just above the thresold, the fixel survives the threshold test.
Also removed multiplie redundant calls to FOD_lobe::finalise().
Identified from discussion in #1228.
@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Jan 8, 2018

I expected to see a difference, as we reasoned before... but there was none. Exactly the same fixels get segmented.

Consistent with the code as it stands: Peak amplitude test occurs before Newton revision of peak, but only the latter uses the precomputer. Have modified so that the peak amplitude test occurs after Newton revision; unlikely to make much of a difference though (without the precomputer bug that is; with, it would make a difference).

I don't know yet, but I already anticipate a possibility of seeing the same thing there...?

No, tckgen wholly uses the precomputer: fod2fixel could only behave that way because it uses both an SH2A transform and a precomputer.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Jan 9, 2018

Consistent with the code as it stands: Peak amplitude test occurs before Newton revision of peak, but only the latter uses the precomputer. Have modified so that the peak amplitude test occurs after Newton revision; unlikely to make much of a difference though (without the precomputer bug that is; with, it would make a difference).

👍 Makes full sense to me now. About the latter change:

This means that if the maximal amplitude value sampled for a lobe from the dixel set is just below the threshold, but Newton revision on the sphere yields an amplitude that is just above the threshold, the fixel survives the threshold test.

I'm not up to date with all the details in the FMLS code, but just checking to be sure: are we certain this doesn't introduce problems on other fronts, such as the -afd output? If a fixel doesn't "have" any dixels in the dixel set, is an integral still successfully computed?
And while I'm typing this, just realised now: does this mean the Newton optimisation now also happens even if no -peak output is requested, whereas (I'm assuming) this wasn't needed/done before? If so, does this increase the computation time significantly if the -peak output is not requested (which would be most scenarios)? And if I'm still correctt(ly guessing) up to this point: is that worth it, if it's unlikely to actually change the outcome significantly?
If some of the previous assumptions are incorrect, and hence it's completely safe/fine to keep this change, then the testing data has to be updated of course. At the moment the tests fail because at least some fixels are now added to the output. With that, I also noticed our tests here are, inherently, very strict: it doesn't matter what precision margin on amplitudes we allow here; if only a single fixel's "inclusion" status changes, the dimensions of the index.mif, directions.mif, etc... change and the test fails (as is the case right now, even though the change was still very subtle in nature).

No, tckgen wholly uses the precomputer: fod2fixel could only behave that way because it uses both an SH2A transform and a precomputer.

👍 Yep, all clear to me now. tckgen is clearly affected by the bug fix, but due to the bug fix implying an amplitude difference of almost 40% here, the thresholds have clearly become inappropriate. I already found them (way) too strict in the past, with all sorts of risks for connectomics and surgical planning; but now it's really bad. I'll provide an example later, but I've settled on strongly arguing for changing the -cutoff default to 0.05. We'll have to be careful though, because -cutoff is also used to specify the FA threshold for tensor tracking, and the default there is coincidentally the same (currently 0.1) there. There's no reason to change the default FA threshold there, so the defaults may now become 2 different numbers. Probably not a bad thing actually, since it's a bit weird that this option's parameters has an entirely different meaning depending on the algorithm selected. Many people overlook this, and confuse FOD thresholds with FA thresholds.

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Jan 9, 2018

Are we certain this doesn't introduce problems on other fronts, such as the -afd output? If a fixel doesn't "have" any dixels in the dixel set, is an integral still successfully computed?

The FOD lbes are segmented and the integral computed based on the amplitudes in the 1281-direction dixel set. So it's impossible for a fixel to not have any dixels within it.

Does this mean the Newton optimisation now also happens even if no -peak output is requested, whereas (I'm assuming) this wasn't needed/done before?

The Newton optimisation has always occurred, regardless of whether -peak is requested, and independently of changes in this branch.

If so, does this increase the computation time significantly if the -peak output is not requested (which would be most scenarios)?

While I don't know exactly how much time this contributes, I doubt it would be the dominant factor compared to the SH2A transformation / level set evolution. Unlike sh2peaks, the Newton optimiser is only executed for those initial points corresponding to the local maxima within the 1281-direction set, which means the initial estimates are both few and already quite close to the actual peaks, and therefore few iterations are required.

Is that worth it, if it's unlikely to actually change the outcome significantly?

It makes sense to me to apply the peak amplitude threshold to the peak amplitude as it is most accurately estimated, not the "nearest guess" that the FMLS obtains. It would also be a bit of code management as to whether or not that peak revision takes place, given it may be triggered not only by use of the -peaks option in fod2fixel but also use of the FMLS code in other contexts.

At the moment the tests fail because at least some fixels are now added to the output.

Yeah I suppose that'll happen if the application of the threshold changes. But given this:

Furthermore, I see fixels with a (wrong) peak amplitude below the 0.1 threshold from the old (dev) version, ...

, I'd suggest the test data should be re-generated.

With that, I also noticed our tests here are, inherently, very strict: it doesn't matter what precision margin on amplitudes we allow here; if only a single fixel's "inclusion" status changes, the dimensions of the index.mif, directions.mif, etc... change and the test fails (as is the case right now, even though the change was still very subtle in nature).

I think I'd advocate leaving that the way it is. No point in putting effort into de-sensitising tests to changes if the high sensitivity is not causing a problem.

I'll provide an example later, but I've settled on strongly arguing for changing the -cutoff default to 0.05

Is this purely for hard threshold CSD? Would we still advocate 0.1 for soft-threshold?

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Jan 9, 2018

The FOD lbes are segmented and the integral computed based on the amplitudes in the 1281-direction dixel set. So it's impossible for a fixel to not have any dixels within it.

Ah, ok. I had failed to appreciate that parts of the lobe below the threshold are also still part of the fixel. In that case, no worries indeed.

The Newton optimisation has always occurred, regardless of whether -peak is requested, and independently of changes in this branch.

Ok, so compared to before, definitely no additional computational load. That's all good then. It'd indeed not be worth the effort and extra complication to make it so, especially not if the computational load of the Newton optimisation isn't expected to be significant. Happy to keep things as is then.

Yeah I suppose that'll happen if the application of the threshold changes. But given this:

Furthermore, I see fixels with a (wrong) peak amplitude below the 0.1 threshold from the old (dev) version, ...

, I'd suggest the test data should be re-generated.

Oh yeah, of course. But they were already re-generated by @jdtournier when the fix was implemented (hence the tests succeeding until right before the most recent commit). But they'll have to be re-generated again after this commit, due to such a "dimension change": essentially a few fixels that now succeed to make the threshold, but didn't before.

I think I'd advocate leaving that the way it is. No point in putting effort into de-sensitising tests to changes if the high sensitivity is not causing a problem.

Fully agreed; I wasn't aiming to change this actually, it was just an observation that I hadn't realised before.

I'll provide an example later, but I've settled on strongly arguing for changing the -cutoff default to 0.05

Is this purely for hard threshold CSD? Would we still advocate 0.1 for soft-threshold?

It's actually a compromise, but a slight shift in favour of multi-tissue / hard thresholded CSD nonetheless. I once proposed a change from 0.1 to 0.05 before, which would've been entirely in favour of the "new" CSDs. However, with the current (almost) overall amplitude changes, there is compensation towards the other end of the spectrum. The coincidentally nice thing is that this allows for a compromise that is still a nice "round" number: 0.05 instead of something like 0.07 or 0.075 or similar.
I'll post a few screen shots in a moment to show the impact.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Jan 9, 2018

Here's an example. Nice multi-shell data and multi-tissue CSD. Tractography seeded from the brain mask and everything else defaults (except -select, I upped that of course) before the bug fix resulted in this:

screenie0001

An OK result, until you look at the details and notice it struggles to reach the cortex in some locations. This could already mean (some) trouble for connectomics (and ACT), and surgical planning if inclusion ROIs are rather strictly confined to the cortex. Increased risk in presence of tumours or lesions, where this could easily lead to connections disappearing, even though they were merely infiltrated by the pathology. Hence my arguments in the past to lower the default already, but I failed to gather enough support for it.

Of course, now that the bug fix results in a 35~40% amplitude change (unless the orientation is very, very closely aligned with the z-axis) that is tested against the threshold, one can imagine what's going to happen... Here it is:

screenie0002

This is really not acceptable. Entire parts are untracked, and streamlines fail to reach the WM-GM boundary in several significant chunks of the cortex. So something needs to change. My proposed new default threshold of 0.05 would result in this:

screenie0003

This overcomes the overall changes brought about by the bug fix, and pushes a little bit further to be somewhat more inclusive. In many, if not most, scenarios I'd still push quite a bit further, but as a default, this isn't so bad.

Here's what you'd get with the current bug fix and my proposed 0.05 default threshold if tracking on FODs from soft-constrained SSST-CSD: (Note that the background is still the multi-tissue result here, so it's easier to directly compare the difference in the streamlines tractograms.)

screenie0004

Would I still advise 0.1 here instead? Well, actually not. To compensate for the bug fix, a threshold of ~0.065 or something would be reasonable. But even then, there's actually no harm at all in using 0.05 for all typical scenarios. If doing connectomics, you've got ACT and parcel assignment anyway to cut short streamlines. If segmenting bundles, e.g. surgical planning or ROI based analysis, you'll typically have 2 inclusion regions on both ends of the bundle at least, which can be augmented with a mask or exclusion regions to deal with a few spurious streamlines. Or even easier, you'd compute a TDI of the bundle defined by only inclusion regions, and threshold the TDI directly (which is what we're doing around here now for studies that need anatomical WM ROIs). Trying to go just of a threshold, you always loose with soft-constrained SSST-CSD anyway, as when you raise the threshold, you start loosing connections through certain crossings. The 3-way crossings first, which sit at important intersections of major bundles.

So really, unless your application is making a desktop background or printed poster of a whole brain tractogram for purely artistic purposes, and you insist on not-using ACT, and you insist on using soft-constrained CSD, and you refuse to optionally look at your threshold yourself a bit for the sake of an artistic cleaned-up output that is a non-critical scenario anyway... well, there's nothing reasonable against an 0.05 threshold at this stage (taking into account the bug fix), I'd say. For any other scenario, one can already resort to at least 2-tissue CSD anyway, which brings in the hard constraint naturally.

Finally, in the spectrum of all scenarios, even soft-constrained SSST-CSD ones, it's almost always still better or safer to be inclusive, rather than risking false negatives. It's the case for connectomics, and segmentations, and in particular for surgical planning.

thijsdhollander added some commits Jan 9, 2018

@Lestropie Lestropie added this to the Milestone 3.0_RC3 milestone Jan 17, 2018

@Lestropie Lestropie referenced this pull request Jan 17, 2018

Merged

Tag 3.0_RC3 #1232

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Jan 17, 2018

Apart from having to change the default -cutoff, when I came across this just now:

http://community.mrtrix.org/t/cutoff-value-in-tckgen/1360

...I was reminded about this:

(quoting myself from some posts above)

I'll provide an example later, but I've settled on strongly arguing for changing the -cutoff default to 0.05. We'll have to be careful though, because -cutoff is also used to specify the FA threshold for tensor tracking, and the default there is coincidentally the same (currently 0.1) there. There's no reason to change the default FA threshold there, so the defaults may now become 2 different numbers. Probably not a bad thing actually, since it's a bit weird that this option's parameters has an entirely different meaning depending on the algorithm selected. Many people overlook this, and confuse FOD thresholds with FA thresholds.

We'll have to be careful documenting the different defaults. We could even consider creating separate -cutoff_... options, e.g. -cutoff_fod and -cutoff_fa... and spit out a warning or error if either option is provided but not applicable with respect to the selected algorithm.

Not per se necessary, but worthwhile considering. I don't feel strongly about it. But I do feel strongly about the actual default value for the FOD cutoff: that really needs to be fixed along with this pull request.

thijsdhollander added some commits Jan 30, 2018

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Feb 9, 2018

@Lestropie : something seems to be wrong with commit 849b475.

Example using a little test FOD image (small region of a template from a collaborator I was QA'ing yesterday; but the problem happened around the whole brain):

For the test case below, I generated fixels using the code just before that commit (i.e. checking out e3c6850):

fod2fixel test.mif test_fixels_before -fmls_peak_value 0.8

...resulting in:

fixelscreenshot0001

This is all good and makes sense. I chose a high threshold of 0.8 for this example, so you can easily visually get an idea of the order of magnitude of the threshold (from the selected fixels in the image, versus the ones that didn't make the cut).

Running the same line of code (i.e. fod2fixel test.mif test_fixels_after -fmls_peak_value 0.8) using the code including commit 849b475 results in:

fixelscreenshot0002

Note that the newly appearing fixels don't make sense, given the order of magnitude of the threshold...

Zooming into the middle voxel to see what's going on:

fixelscreenshot0003

...and from another angle:

fixelscreenshot0004

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Feb 10, 2018

OK, clearly I'm being too lenient on the Newton optimiser... it's jumping from seeds on those small peaks onto the adjacent large lobe, and hence those tiny ones are surviving the peak amplitude threshold. The revised peak will need to be tested to ensure that the dixel closest to the newly-selected peak is a member of that particular lobe. Bit of fiddling, but shouldn't be too difficult.

thijsdhollander and others added some commits Feb 12, 2018

FOD FMLS segmenter: Fix peak revision
When the initial FOD peak identified via the FMLS process is then revised via Newton optimisation, apply additional constraints to decide whether or not the outcome of Newton optimisation should be trusted:
- The dixel to which the new peak direction is closest must be a part of the same FOD lobe.
- The new peak direction must be closer to the initial peak direction (that was used as the seed point for the Newton optimisation) than to any other FOD peaks within the same lobe.
Addresses issue introduced in commit 849b475 as reported in #1228.

thijsdhollander and others added some commits Feb 13, 2018

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 11, 2018

OK, I have a working script that:

  • For:
    Single-shell single-tissue soft-constraint CSD
    Single-shell 2-tissue CSD (i.e. WM & CSF using b=0 & b=3000)
    Multi-shell 3-tissue CSD:

    • For:
      Tracking without ACT
      Tracking with ACT but without back-tracking
      Tracking with ACT & back-tracking:

      • For a range of FOD amplitude thresholds:

        • Generates tracks
        • Takes an appropriate screenshot

So, this will give the ultimate suite of data with which to choose what we want to do taking into account a range of use cases. Hopefully tomorrow I'll post one mighty big image 😈


It would also be worth having some side-by-side comparison of tracking with & without the bug fix, which might show not only the difference in effective threshold but also the Z-axis bias.

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 12, 2018

Matrix of tracking outcomes:

tracking_thresholds_small

Full-resolution image here.

Don't know if this provides the specific information we need; if there are any alternative ideas (e.g. generating a similar image matrix for targeted tracking), those can be investigated also.

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 12, 2018

Also because I tried to fit the thalamus into the FoV / didn't want to zoom out too far / didn't show the DWI-based brain mask, can't see whether or not streamlines are making it to the edge of the mask when ACT isn't used. I'll keep playing around, but ideas more than welcome.

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 12, 2018

Also: While we can't trivially alter the default threshold based on FOD reconstruction algorithm, we can trivially alter it based on the presence or absence of ACT (if we choose to).

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 16, 2018

Second attempt: 0.01 increments, used GM seeding, and more tracks.

drawing_small

Full-resolution here.

tckgen: Revise default cutoff
The default threshold for tracking is now dependent on the particular algorithm selected.
@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 17, 2018

4a949ea appears to be doing as it should. Testing it did however remind me of #930.

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 17, 2018

Even though fod2fixel wasn't affected by the SH precomputer bug (due to its own counteracting bug), do we need to discuss altering the default peak amplitude threshold there as well? Docs on dev currently have separate recommendations for single- and multi-tissue CSD regarding the population template, but use the default for subject segmentations, which is still 0.1 at present.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Apr 17, 2018

Yep, that's all (still) deliberate. Those altered recommendations (compared to the default value) take into account the fact that it runs on a template at that stage in the pipeline. For single-tissue CSD at that stage, a slightly higher threshold is safer to not end up with lots of false positive fixels in the GM (tractography, on the other hand, isn't that much affected by all of those, since many don't align with the nearby WM). For multi-tissue CSD, we've always tuned these manually, and I've assisted different people manually with their template fixel mask at this stage. 0.06 came out as a safe threshold, that also lands the number of fixels in an acceptable/realistic range. This has/had to be lower than the subject thresholds also for the reason that the template typically has lower FOD peak amplitudes.

For subject segmentations, again we want to be a little bit more careful compared to tractography, since all those fixels (may) end up in the analysis; at the same time, we can comfortable be safer than tractography, since in FBA we mostly need the more robust core WM. Due to variance of cortical topology, we can't find significant results there anyway most of the time; so it's not that problematic that no fixels at the subject level are extracted all the way up until (in) the cortex.

In general, in tractography we've also got other constraints (such as curvature) that can inherently deal with a noisy peak here and there, as long as it's not consistent with surrounding structure. For most applications where we segment fixels individually per voxel, we may want to be slightly more aggressive on cleaning up noisy peaks.

Long story short: those template recommendations are definitely optimised now, based on a wide range of templates in different studies... I definitely wouldn't touch those. Furthermore the updated docs for both FBA pipelines have a big warning box at that step, which explains further (to great extent) how to deal with those values. The respective values are very good starting points, that seem to generalise very well, but the advise there is again to go hands-on for the best result.

As to the default value used within fod2fixel itself: I'd personally keep it at 0.1, but if we feel unsure about that one, we could generate another one of those massive images (as done for the tractography; but just 3 rows, 1 for each CSD type) in principle, if we would want to have a better look at it.

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 18, 2018

fod2fixel2 peak threshold is tough to assess. Showing ODFs / fixels was just way too messy. Here's approximately the same FoV as the images before, showing the number of fixels in each voxel (white is >= 6):

drawing

Random thoughts:

  • While the case for using a peak amplitude threshold for population template segmentation makes sense (require consistency in alignment across subjects to be included in fixel mask), for subject-wise segmentation there is the option of thresholding on the integral instead / in addition to.

  • While more fixels looks messier, if the purpose of the segmentation is to subsequently do fixelcorrespondence, then the implications for that process become important:

    • If a lower threshold introduces new fixels, that will only be problematic if one of those fixels is more closely aligned to a template fixel that otherwise a larger subject-space fixel would have been assigned to.

    • If a higher threshold removes fixels, that could increase variance in the context of FD & FDC, as the subject would obtain a value of 0 rather than what the value in the fixel would have been.

    • (This may change in the future as I have an improved fixelcorrespondence, but we'll deal with the current situation for mow)

This is probably too sensitive to both the FOD estimation algorithm and the context in which fod2fixel is being used to have an unambiguous default... maybe even more so than the tckgen case. Posting nonetheless in case anybody has thoughts or feelings.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Apr 18, 2018

Even taking all those bullet points into account, looking at those results, I'd definitely still happily stick to 0.10 there. Even for 3-tissue CSD, I already see a (few) false positives using 0.1, and more for 0.05. For 2-tissue and single-tissue (soft) CSD, things really become too crazy, I'd argue.

Side note:

If a lower threshold introduces new fixels, that will only be problematic if one of those fixels is more closely aligned to a template fixel that otherwise a larger subject-space fixel would have been assigned to.

You'd be surprised how easily that happens, especially in voxels with genuine 2- or 3-way crossings. The purpose of fixelcorrespondence to begin with is to overcome an extent of angular misalignment of fixels (vs template). The more false positive peaks are present, and given the current mechanism of fixelcorrespondence at least, the more likely the wrong ones get assigned to the template fixels for smaller angular misalignments.

Happy to keep things the way they are for fod2fixel currently. The result is, on average, pretty good for 3-tissue CSD (and unavoidably worse for single-tissue CSD); also for applications that don't involve applying fixelcorrespondence subsequently.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Apr 24, 2018

@MRtrix3/mrtrix3-devs , any further opinions? If not, I reckon we're done here.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Apr 25, 2018

This is entirely ready to hit the button. I'm not at work now (ANZAC day down under), but unless anyone comments in the next 20 hours, I'll consider this good to go. If anyone's not happy/comfortable with that, speak up.

@jdtournier jdtournier merged commit 39947db into dev Apr 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jdtournier jdtournier deleted the fix_SH_precomputer branch Apr 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment