Skip to content

minimum/maximum value of the all-positive/negative data#12383

Merged
larsoner merged 12 commits intomne-tools:mainfrom
withmywoessner:detect_min_max
Jan 24, 2024
Merged

minimum/maximum value of the all-positive/negative data#12383
larsoner merged 12 commits intomne-tools:mainfrom
withmywoessner:detect_min_max

Conversation

@withmywoessner
Copy link
Copy Markdown
Contributor

Reference issue

#12381

What does this implement/fix?

Currently, evoked.get_peak() can't find a min/max in all positive/ all negative data.
Implementation:
evoked.get_peak(mode="neg", strict=False)

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

mne/evoked.py Outdated
tmax : float | None
The maximum point in time to be considered for peak getting.
mode : {'pos', 'neg', 'abs'}
mode : {'pos', 'neg', 'abs', 'min', 'max'}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confused; this doesn't match the API in the PR description (as was discussed in #12381)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that! I didn't push my changes yet.

@drammock
Copy link
Copy Markdown
Member

I see that you've marked this as draft and I shouldn't have been so hasty to review. FYI: you can mark as draft when first creating the PR (the green "create pull request" button has a dropdown arrow that switches the button to "create draft pull request"). If you open a regular PR first and then later convert to draft, a bunch of people are automatically subscribed to activity on this PR, and converting to draft mode later doesn't unsubscribe them. It's a quirk of how GitHub works and it often leads me to review work that isn't ready for review because it shows up in my inbox (which it wouldn't if the PR were made in draft mode initially).

@withmywoessner
Copy link
Copy Markdown
Contributor Author

withmywoessner commented Jan 23, 2024

I see that you've marked this as draft and I shouldn't have been so hasty to review. FYI: you can mark as draft when first creating the PR (the green "create pull request" button has a dropdown arrow that switches the button to "create draft pull request"). If you open a regular PR first and then later convert to draft, a bunch of people are automatically subscribed to activity on this PR, and converting to draft mode later doesn't unsubscribe them. It's a quirk of how GitHub works and it often leads me to review work that isn't ready for review because it shows up in my inbox (which it wouldn't if the PR were made in draft mode initially).

Okay, thanks for letting me know! I'll be sure to do that next time. Github should let people set up a grace period for notifications. It probably wastes a lot of maintainers' time.

@withmywoessner withmywoessner marked this pull request as ready for review January 23, 2024 22:47
Copy link
Copy Markdown
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me, just some minor stuff

withmywoessner and others added 2 commits January 24, 2024 11:32
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@withmywoessner
Copy link
Copy Markdown
Contributor Author

Done! @larsoner Thank you

@drammock
Copy link
Copy Markdown
Member

@larsoner I let you merge if happy

@larsoner larsoner merged commit 03d78f4 into mne-tools:main Jan 24, 2024
@larsoner
Copy link
Copy Markdown
Member

Thanks @withmywoessner !

@withmywoessner withmywoessner deleted the detect_min_max branch January 24, 2024 18:44
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
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