Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update prep4phys #23

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Update prep4phys #23

wants to merge 25 commits into from

Conversation

me-pic
Copy link
Collaborator

@me-pic me-pic commented Oct 30, 2024 •
edited by smoia
Loading

Integrate latest peakdet changes in prep4phys

Proposed Changes

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

Checklist before review

  • I added everything I wanted to add to this PR.
  • [Code or tests only] I wrote/updated the necessary docstrings.
  • [Code or tests only] I ran and passed tests locally.
  • [Documentation only] I built the docs locally.
  • My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
  • My code respects the adopted style, especially linting conventions.
  • The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
  • I added or indicated the right labels.
  • I added information regarding the timeline of completion for this PR.
  • Please, comment on my PR while it's a draft and give me feedback on the development!

pre-commit-ci bot and others added 24 commits August 5, 2024 21:28
@pre-commit-ci
[pre-commit.ci] pre-commit autoupdate
0ce2395 
updates:
- [github.com/psf/black: 24.4.2 → 24.8.0](psf/black@24.4.2...24.8.0)
- [github.com/pycqa/flake8: 7.1.0 → 7.1.1](PyCQA/flake8@7.1.0...7.1.1)
@me-pic
Add contribution
730fb8d
@smoia
Merge pull request #81 from me-pic/add/contribution
e7b1f0a 
Add contribution
@smoia
Update Zenodo (and hopefully not breaking it)
ce23f72
@smoia
Add @maestroque as a contributor
6b5fe5a
@smoia
Add @RayStick as a contributor
8daf87a
@smoia
Add @afni-dglen as a contributor
d48a9a8
@smoia
Add @RayStick as a contributor
955f88f
@smoia
Add @me-pic as a contributor
bae43b0
@smoia
Update @me-pic as a contributor
4a2a8e3
@smoia
Add @m-miedema as a contributor
f887e7e
@smoia
Update @smoia as a contributor
b275d53
@smoia
Update @smoia as a contributor
2f93e80
@smoia
Update @afni-dglen as a contributor
f306a1e
@smoia
Update contributors
2c95155
@smoia
Update readme
ebb2add
@smoia
Merge pull request #82 from smoia/int/zenodo
250f7c7 
Update contributors
@smoia
The added peak type is a standard python int
fe4a620
@smoia
Merge pull request #79 from smoia/fix/history
1ebf46a 
Fix type errors in exporting history after manually adding peaks
Update CHANGELOG.md [skip ci]
4d9523c
@smoia
Merge pull request #75 from physiopy/pre-commit-ci-update-config
75fa443 
[pre-commit.ci] pre-commit autoupdate
@smoia
Make sure peak detection works with negative signals
a2b7e8c
@smoia
Merge pull request #84 from smoia/fix/negsignal
1acd337 
Make sure peak detection works with negative signals
Update CHANGELOG.md [skip ci]
e99c705
@github-actions github-actions bot added Documentation This issue or PR is about the documentation Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog labels Oct 30, 2024
smoia
smoia reviewed Oct 30, 2024
View reviewed changes
peakdet/operations.py Outdated Show resolved Hide resolved
smoia
smoia reviewed Oct 30, 2024
View reviewed changes
peakdet/operations.py
locs, heights = signal.find_peaks(data[:], distance=cdist, height=thresh)

# check if data is negative, if so make it all positive and continue with signal
phys_signal = data.data - data.data.min() if data.data.min() < 0 else data.data
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that maybe we can just always work with data at min=0 for peak detection (it doesn't rescale the data for real). That should make percentages for thresh easier to work with. Thoughts?

Suggested change
phys_signal = data.data - data.data.min() if data.data.min() < 0 else data.data
phys_signal = data.data - data.data.min()

smoia
smoia approved these changes Oct 30, 2024
View reviewed changes
Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

See the suggestion on min()=0, for the rest all good, thanks!

@smoia smoia added the Minormod This PR generally closes an "Enhancement" issue. It increments the minor version (0.+1.0) label Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smoia smoia smoia approved these changes

Assignees

@me-pic me-pic

Labels
Documentation This issue or PR is about the documentation Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog Minormod This PR generally closes an "Enhancement" issue. It increments the minor version (0.+1.0)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@me-pic @smoia