While I was looking into this, line 104 in the FMLS FOD segmenter struck me as suspicious:

 99           void revise_peak (const size_t index, const Eigen::Vector3& real_peak, const default_type value)
100           {
101             assert (!neg);
102             assert (index < num_peaks());
103             peak_dirs[index] = real_peak;
104             if (!index)
105               max_peak_value = value;
106           }

Why is that check on line 104 in there? I can't think of any reason why we would only revise the peak value for the first peak... Any everything seems to work fine if I comment out that line. Can anyone shed any light on this...?

• We'll need to draft a forum announcement for this change.

• The fod2fixel tests also need revision.

• With that change, macro SH_NON_M0_SCALE_FACTOR is not used. So my guess is that if the non-orthonormal SH basis is used, something in here is going to misbehave...

Why is that check on line 104 in there?

This only revises the maximum peak value in the case where there are multiple peaks per lobe. index here refers to the peak index within the fixel, not fixel index within the voxel.

By default this never does anything interesting, because FMLS_RATIO_TO_PEAK_VALUE_DEFAULT is 1.0. This means that if there are multiple peaks detected within a lobe, even if the trough between them is tiny, they will be separated into multiple fixels. However if the -fmls_peak_ratio_to_merge option is provided, then multiple peaks may be merged to form a single fixel, depending on the relative amplitude between the trough and the smaller peak.

OK, line 104 all makes sense, no problem.

Otherwise, yes this will warrant a new release and an announcement to go with it. What I'm not sure about is how big an impact this will have had on tractography - and hence how big a deal to make of it in the announcement. My guess is the difference before and after is likely to be largely negligible, only impacting on minor directions close to the 0.1 threshold. I'm not sure how to even assess this, to be honest...

Otherwise, maybe it's time to drop support for the non-orthonormal basis altogether...? The SH_NON_M0_SCALE_FACTOR macro is in fact used elsewhere (in the ODF renderer code), so I haven't removed it. But if we get rid of it, that'd be one less thing to worry about...

OK, just updated the test data from a fresh fod2fixel run. Should be good to go. But given that we're talking about making this a fresh release, I suggest we tidy up whatever pull requests are open on dev and merge this into it, and eventually back onto master at release time...

would be fine with dropping support for the non-orthonormal basis

Right, in the spirit of my previous comment, it might make sense to edit this pull request to merge to dev, so that we can clean up dev in preparation for an RC3 release later in the week. Alternative would be to merge this to master now, and then merge dev when it's ready later, but somehow I find that a bit messier in terms of tracking what version people are using. Happy to over-ruled - opinions welcome as always...

In general: I'm ok with either option... but I do feel (very) strongly about the -shell(s) to -bvalues name change that was still under discussion (?). I'm very keen on pushing through with that one before dev goes to master, since a partial name change from -shell to -shells already happened on dev (which I don't agree with so much, unless a final name change to -bvalues happens). I'd rather only see this name being changed once (and for all, decently). I'll try and find that discussion somewhere later today; better not to have it in this pull request to avoid confusion.

Apart from that, it'd be good if we take a little bit of time (a couple of days or so) to carefully think through the impact of this fix, so we don't need to patch up default parameters and documentation later on. Let's get it right in one go, I say. About that, these are just a couple of things that come to mind, but I may be overlooking much more:

• First there's the "relative" bias that was there before, and gets resolved now. What I mean by that is the difference in amplitude (biased) evaluation that was orientation dependent. From @jdtournier and my little experiments in fod2fixel -peaks overestimates certain peak amplitudes #1204, we essentially learn that it's a strong relative effect only for orientations aligning quite closely to the z-axis, versus all the rest. "all the rest" among them selves didn't vary (due to bias) that much. Essentially, dwi2response tournier was probably less likely (than it should've been) to select single-fibre voxels where the orientation aligned closely to the z-axis. That's not really a big issue (as there's many good single-fibre voxels all over the place anyway...). For tractography, the impact is hopefully limited mostly to scenarios where people where not using SIFT(2), as SIFT(2) will undo most of the bias (even due to the tractography method) any way. Only effect still remaining is whether certain structures dropped below/above the threshold for tracking of course. Hard to quantify, but I suppose the impact wasn't all that drastic... well, I'm hoping at least.

• But next, there's also the "absolute" bias that gets resolved now. Here it's important to note that most of the domain (apart from aligned to z-axis orientations) was overestimated in amplitude by about 30-40%. That's important to take into account with this change, as most amplitudes will now be 30-40% lower. We need to take this into account for certain default values! The first one that comes to mind is the FOD -cutoff value in tckgen. It seems to me that, if we want to retain a similar-ish default behaviour, its default value would also have to be lowered at least 30-40% (maybe to at least 0.05, because more and more people use multi-tissue CSD)? But it might be worthwhile to check other defaults, both defaults in certain commands(' options), as well as in the userdocs (i.e. "default" values for certain pipelines). Let's think this through carefully before anything ends up on master and in the announcement; so we don't cause more confusion than need be.

• Any other stuff I'm overlooking?

All good points. I've already commented on the -shells / -bvalues in #1211, so I'll leave that for now.

Otherwise, that's a great analysis on the impact of tractography: indeed with SIFT, any bias should be minimised. But there will still be a minor effect. Not convinced it would be easily detectible, but I don't think we can argue that it won't be there.

I'm not too sure what to do about the -cutoff issue though. I find it surprising that if anything, we've not seen bigger differences between version pre- and post- SH basis change. And if anything, I've been tempted to reduce the cutoff, which is the opposite of what this might suggest... I guess we should run a few experiments with and without the fix and see whether we can detect any meaningful difference...?

But there will still be a minor effect. Not convinced it would be easily detectible, but I don't think we can argue that it won't be there.

Fully agreed, I definitely wouldn't want to argue it isn't there. That was mostly me just hoping results aren't too drastically affected (but again, this feels quite complicated to truly quantify properly). But as to external communication we should be very clear on the nature of the bias that was effectively there.

I find it surprising that if anything, we've not seen bigger differences between version pre- and post- SH basis change.

Yes, I'm a bit confused about this too...

And if anything, I've been tempted to reduce the cutoff, which is the opposite of what this might suggest...

😕 ...yeeeesss... that's what I was also suggesting ❓ There was an overestimation for most of the angular domain's amplitudes before, which is now fixed. This fix now results in amplitudes that are 30-40% lower, so if we don't want this fix to introduce a drastic change in tracking behaviour (as in: the cutoff criterium would otherwise inherently become way to strict), we need to lower the default for -cutoff. It is/was 0.1 before, hence I'm suggesting to for instance lower it to 0.05. That's a bit more (lowered) than strictly needed (50% rather than 30-40%), but given more widespread MSMT-CSD usage (with the hard constraint and all), a lower threshold is what I was constantly already advising to users; entirely in line with the actual MSMT-CSD paper.

So TODO here:

• check the actual fix itself or course
• check default tckgen FOD cutoff; as indicated above, I think this should consequently be lowered
• just thought of now: probably same thing for the default -fmls_peak_value in fod2fixel
• in case the previous bullet is indeed a thing, lower the "default" recommendations for this parameter in both (single tissue and multi tissue) FBA pipelines in the userdocs
• potentially similar changes to such values in other userdocs pages
• others...?

So well, we really need to be careful here ⚠️ ⚠️ ; this one has a severe risk of altering peoples' pipeline behaviours in a very unwanted way. The impact of the aforementioned -fmls_peak_value when generating the fixel analysis mask in an FBA for instance, has been found (by me, in every FBA I've been involved in) to have a very, very, very significant effect on the final outcome of the FBA; due to how CFE works.

• check the actual fix itself or course

OK, I've just pushed a minor change to sh2peaks to allow it to use the precomputer via the -fast option, and added an additional test for it, so it checks out with the same input and validation data both with and without -fast (with reduced tolerance for -fast). I think that should give us confidence that it's OK...?

• check default tckgen FOD cutoff; as indicated above, I think this should consequently be lowered

So I'm not sure what to think here... I think I agree that it needs to be lowered, what I don't understand is why this wasn't apparent before. This bug was introduced as part of the updated_syntax dump, and should have caused more spurious streamlines in the output after the update - since the threshold was effectively lower than expected compared to the FOD amplitude. To counter those effects, we would have needed a higher threshold to counter the bug - yet if anything, many of us have already been using lower threshold values than the default. What's going on...?

• just thought of now: probably same thing for the default -fmls_peak_value in fod2fixel

I probably agree, but that will definitely need to be spelled out as a major change, given your later comment about FBA analyses...

• in case the previous bullet is indeed a thing, lower the "default" recommendations for this parameter in both (single tissue and multi tissue) FBA pipelines in the userdocs
• potentially similar changes to such values in other userdocs pages
• others...?

👍

So well, we really need to be careful here ⚠️ ⚠️ ; this one has a severe risk of altering peoples' pipeline behaviours in a very unwanted way. The impact of the aforementioned -fmls_peak_value when generating the fixel analysis mask in an FBA for instance, has been found (by me, in every FBA I've been involved in) to have a very, very, very significant effect on the final outcome of the FBA; due to how CFE works.

OK, this is going to need a bit of discussion. Maybe you can all report what values you've found to work best, and we can come to consensus as to what the best value would be before this fix. We'd then need to agree on what it should change to given the fix...

Ok, rebased to dev, and merged dev inhere, both on the main as well as the test_data repo. To make our test_data repo construct not break down in combination with GitFlow, there's a dev branch on there since a little while as well. Don't forget to merge back to dev on the test_data repo as well when eventually this pull request result in a merge to dev.

See the test_data repo's network for overview: https://github.com/MRtrix3/test_data/network

Ok, that's it for this one for today. Happy holidays, and see you all again in 2 weeks!

Throwing this into the mix since it's slightly relevant to the discussion RE: selection of tracking threshold.

A while ago I started playing with some ideas for changes to the behaviour of tracking. I never finished them, but one thing I did try was altering the behaviour of the iFOD2 calibrator with respect to the FOD amplitude threshold. Details here (private GitLab repo link). Just in case it's worth experimenting with this as well given the behaviour here is going to "change" with this merge as it is.