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.

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

[Fix] refactor ecg_findpeaks #973

Merged
merged 10 commits into from
Mar 24, 2024

Conversation

danibene
Copy link
Collaborator

@danibene danibene commented Mar 10, 2024

Description

This PR aims to refactor ecg_findpeaks() following a failing codeclimate check: https://codeclimate.com/github/neuropsychology/NeuroKit/pull/926

Avoid deeply nested control flow statements.

                    for missed_peak in missed_peaks:
                        if missed_peak - peaks[idx[-2]] > int(0.36 * sampling_rate) and ma[missed_peak] > 0.5 * th:
                            insort(QRS, missed_peak)
                            break

Severity: Major
Found in neurokit2/ecg/ecg_findpeaks.py - About 45 mins to fix

Proposed Changes

I changed the ecg_findpeaks_hamilton() function so that it is broken up into smaller functions. I also added a regression test for it.

Checklist

Here are some things to check before creating the PR. If you encounter any issues, do let us know :)

  • I have read the CONTRIBUTING file.
  • My PR is targeted at the dev branch (and not towards the master branch).
  • I ran the CODE CHECKS on the files I added or modified and fixed the errors.
  • I have added the newly added features to News.rst (if applicable)

@danibene danibene marked this pull request as draft March 10, 2024 23:51
@DominiqueMakowski
Copy link
Member

Nice, though if need be we can also just discard that check on codeclimate - I wonder whether you have access to codeclimate's NK checks if you log in with ur GH account

@danibene danibene marked this pull request as ready for review March 12, 2024 02:45
@danibene
Copy link
Collaborator Author

Couldn't find that option - would it make sense to update the config file in the repository?

@DominiqueMakowski
Copy link
Member

Couldn't find that option - would it make sense to update the config file in the repository?

Yeah to be fair I think we should drop codeclimate entirely... it was kinda useful years ago but now with all the GH actions not so much

@DominiqueMakowski
Copy link
Member

can I merge this?

@danibene danibene merged commit 8e27aef into neuropsychology:dev Mar 24, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants