mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

Adding ability to get minimum and maximum peaks from evoked object

Open withmywoessner opened this issue 1 year ago • 5 comments

Describe the new feature or enhancement

Adds the ability to find minimum and maximum peaks not just negative and positive.

Describe your proposed implementation

Append a simple conditional to the _get_peak() function. Usage: evoked.get_peak(mode="max")

Describe possible alternatives

N/A

Additional context

No response

withmywoessner avatar Jan 23 '24 05:01 withmywoessner

not sure I understand what you're seeking here:

  • evoked.get_peak("pos") returns the time of the most extreme positive value
  • evoked.get_peak("neg") returns the time of the most extreme negative value
  • evoked.get_peak("abs") returns the time of the most extreme value (positive or negative, whichever has largest absolute value)
  • evoked.get_peak("max") returns ???
  • evoked.get_peak("min") returns ???

drammock avatar Jan 23 '24 16:01 drammock

Hey @drammock! I think the get_peak("pos") just considers positive values and get_peak("neg") just considers negative. That means that if you have only positive values there is no way to find the minimum of the positive values. See this forum post: https://mne.discourse.group/t/can-get-peak-function-specify-the-maximum-or-minimum-value/2673 image

withmywoessner avatar Jan 23 '24 17:01 withmywoessner

ok, I understand now.

The suggested work-around provided in that forum post gives the per-channel minima and is only 2 lines:

idx_min = np.argmin(evoked.data, axis=1)  # min for each channel
t_signal_min = evoked.times[idx_min]

Going from that to the global min isn't much more:

ch_idx, time_idx = np.unravel_index(np.argmin(evk.data), shape=evk.data.shape)
ch_name = evk.ch_names[ch_idx]
min_time = evk.times[time_idx]
min_amp = evk.data.min()  # or evk.data[ch_idx, time_idx]

If I had to guess, the method was written the way it is because it's designed to test a scientific assumption (something like "in this condition there ought to be a negative extremum around N milliseconds") and in a context like that it is arguably a good thing to fail if the input evoked is all positive. The use case of "which channel has the smallest positive value in some all-positive data" was (I guess?) not judged to be useful / frequent enough to need a convenience method.

So I guess my questions to you @withmywoessner are:

  1. is this a frequent analysis need?
  2. are you doing this in a context where the other stuff done by get_peak (i.e., merging grad channels) is necessary?
  3. is the equivalent numpy code above too complicated / too hard to discover / otherwise inadequate?

If we're going to alter how this method works, I'm a bit hesitant to have options both "pos" and "max" (or "neg" and "min"), they seem too similar and thus likely to cause confusion. I'd rather have something like evk.get_peak(..., mode="neg", on_sign_mismatch="info"|"warn"|"raise") so that users could disable the constraint that says "error if asked for neg and data are all pos". Also open to other API ideas.

drammock avatar Jan 23 '24 17:01 drammock

  1. is this a frequent analysis need?
  2. are you doing this in a context where the other stuff done by get_peak (i.e., merging grad channels) is necessary?
  3. is the equivalent numpy code above too complicated / too hard to discover / otherwise inadequate?
  4. is the equivalent numpy code above too complicated / too hard to discover / otherwise inadequate?

If I wanted to the restrict the time-ranges considered to detect peaks of event related potentials. I think the numpy code would be not that complicated, but for someone like me who is not that adept in numpy may be a bit confusing. The code itself uses a mask to do this I think?

    if tmin is None:
        tmin = times[0]
    if tmax is None:
        tmax = times[-1]

    if tmin < times.min() or tmax > times.max():
        if tmin < times.min():
            param_name = "tmin"
            param_val = tmin
        else:
            param_name = "tmax"
            param_val = tmax

        raise ValueError(
            f"{param_name} ({param_val}) is out of bounds. It must be "
            f"between {times.min()} and {times.max()}"
        )
    elif tmin > tmax:
        raise ValueError(f"tmin ({tmin}) must be <= tmax ({tmax})")

    time_win = (times >= tmin) & (times <= tmax)
    mask = np.ones_like(data).astype(bool)
    mask[:, time_win] = False 

I did not want to deal with this in my own script so I just ending up using pandas before making this change. The evk.get_peak() is very convenient as it already allows you to restrict the time intervals: get_peak(tmin=.500 tmax=.800, mode='min', return_amplitude=True)

I'd rather have something like evk.get_peak(..., mode="neg", on_sign_mismatch="info"|"warn"|"raise") so that users could disable the constraint that says "error if asked for neg and data are all pos". Also open to other API ideas.

That sounds good. @drammock

withmywoessner avatar Jan 23 '24 18:01 withmywoessner

ok, so the other stuff done by get_peak() is needed. Would this API work / make sense for you? Seems cleaner than what I suggested before

evoked.get_peak(mode="neg", strict=False)

where new keyword arg strict determines whether to error out if mode="neg" and there are no negative values (the current behavior, would become the strict=True behavior), or instead to happily return the minimum value of the all-positive data (the strict=False behavior).

drammock avatar Jan 23 '24 21:01 drammock