fod2fixel: Revise options / help / code RE: "peaks"

[Lestropie]

This kept irking me following discussion in #1874. Going back and looking through the code it was clear that this ambiguity of the word "peak" has been there for a while. Gets worse when talking about things like lobes containing multiple peaks, or merging lobes based on the ratio between the minimum of maximal amplitudes and the amplitude at the bridging dixel...

I had a go at making some changes to try to remove the ambiguity; not wholly sold on all of it (hence draft PR), but there's at least some stuff in there to consider.

[jdtournier]

Had a quick look. First impression is I'm not at all keen on this... I think in most places it's less explicit than it used to be, and what's more, I stand by my original comment on the matter:

Personally, I think the main thing is for us to express what these commands generally expect to operate on, with the understanding that anything that 'looks like' a peaks image (in this instance) is acceptable.

I really don't see the need to change anything here...

[Lestropie]

I stand by my original comment on the matter

It's not quite the same issue as commands operating on what's being referred to colloquially as a "peaks image", which is just an agreed term for image data of a specific format. Throughout the relevant code the word "peak" is currently used somewhat interchangeably to refer to a direction / maximal amplitude / both, and usage of clarifying terms to disambiguate is inconsistent. There might be some benefit to the readability of the code in choosing a standard and sticking with it. I'm less convinced about interface changes... but there is a conflict between having "peaks" refer to fixel data in the old-style 4D 3N image format, and fod2fixel -peak returning data not of that format, instead differentiation of that option from the other outputs of that command being specifically about amplitudes and not affecting directions.

[jdtournier]

Ok, so there's more levels than I'd appreciated. So if I understand correctly, the problem is that 'peak' was used to refer to either the peak direction or peak amplitude depending on context? And on top of that, that the format of the inputs/outputs can take different forms depending on the command...?

I can see the rationale for fixing the amplitude/direction ambiguity, but I'm not sure I'd change the API just to address that - unless it really makes sense. For instance, changing peak_value to max_value doesn't really seem to help much IMO. I think peak_amplitude would be better if you have to change anything, but personally I think it's fine as-is. And I think that applies to the bulk of the changes: I don't think max is a good replacement for peak if you're trying to make it more obvious that it's referring to the amplitude of the peak. I reckon the two terms (max and peak) can be used almost interchangeably, so I don't think it helps disambiguate.

Of the changes you're proposing, I reckon fod2fixel's -peak option is pretty clear given the current docs. But -max is fine too. I'd be tempted to go for -peak_amplitude, which is IMO clearer, but also allows current usage without change.

I'll go through your changes now and make specific comments against the relevant lines.

[jdtournier]

Ok, have a look at my individual comments, see what you think. Main point is I don't think the word peak is the problem here, and replacing it with max doesn't IMO clarify which aspect of it (amplitude or direction) is being considered. I think I'd rather keep talking about the 'peak', but be stricter about specifying amplitude or direction...

[thijsdhollander]

I can see the rationale for fixing the amplitude/direction ambiguity, but I'm not sure I'd change the API just to address that - unless it really makes sense. For instance, changing peak_value to max_value doesn't really seem to help much IMO. I think peak_amplitude would be better if you have to change anything, but personally I think it's fine as-is. And I think that applies to the bulk of the changes: I don't think max is a good replacement for peak if you're trying to make it more obvious that it's referring to the amplitude of the peak. I reckon the two terms (max and peak) can be used almost interchangeably, so I don't think it helps disambiguate.

I agree. A peak is the same as a (local) maximum. It has a position in the domain and an amplitude. The position in the domain is an orientation for antipodally symmetric spherical functions, or a direction if you want to allow spherical functions in general.

Whether the orientation/direction or amplitude is expected when the term "peak" pops up, might often be clear from the context; but this is subjective. In my experience, I haven't seen any users so far struggle with this though, so it appears the context must have been informative.

[Lestropie]

If "maximum" is no less ambiguous as being in reference to the amplitude rather than the position than is "peak" (personally I think it is, but only a little), then is there an alternative? Otherwise I'll probably add the distinction throughout the code, then think about at the API.

[jdtournier]

I agree 'maximum' leans towards an interpretation as 'amplitude', in the same way that 'peak' leans towards direction/orientation - but I don't think it resolves any ambiguity that wasn't already there. I think if you want to make any changes, you'll need to mention amplitude or orientation explicitly in there. I can't think of an alternative terminology that would carry that distinction in fewer characters...

And personally, between 'peak' and 'maximum', I much prefer 'peak' to describe what it is whose direction or amplitude we're referring to...

[Lestropie]

I don't intend on making any further modifications here of my own accord. Changes to interface are reasonably small, but comments prior to merge still welcome.