0.3.0 - #1179
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on maintenance and minor improvements to the NeuroKit2 library. It updates the pandas dependency to ensure compatibility with newer versions and refines data processing logic in Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on updating the pandas dependency to version 2.0.0 or higher. The changes made in read_acqknowledge.py and epochs_create.py are appropriate adjustments for compatibility with the new pandas version, such as using ffill() instead of the deprecated fillna(method='pad') and including the 'string' dtype in type selections. I have one minor suggestion to use an f-string for better code readability when constructing an error message.
| "NeuroKit error: read_acqknowledge(): couldn't find the following file: " | ||
| + filename |
There was a problem hiding this comment.
For improved readability and consistency with modern Python practices, consider using an f-string for formatting the error message.
| "NeuroKit error: read_acqknowledge(): couldn't find the following file: " | |
| + filename | |
| f"NeuroKit error: read_acqknowledge(): couldn't find the following file: {filename}" |
There was a problem hiding this comment.
Pull request overview
This PR appears to be part of the v0.3.0 prep work, primarily updating pandas compatibility/requirements and making small pandas-2-friendly adjustments in epoch creation and AcqKnowledge import.
Changes:
- Update core dependency constraint to require
pandas>=2.0.0. - Adjust
epochs_create()dtype sanitization to also consider pandasstringdtype columns. - Replace deprecated
fillna(method="pad")usage with.ffill()inread_acqknowledge().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pyproject.toml |
Updates pandas dependency requirement to >=2.0.0. |
neurokit2/epochs/epochs_create.py |
Updates dtype sanitization logic to include string dtype columns. |
neurokit2/data/read_acqknowledge.py |
Switches to .ffill() for forward-filling missing samples (pandas 2 friendly). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ).items(): | ||
| # Check whether columns are indices or label/condition | ||
| values = column.unique().tolist() | ||
| zero_or_one = all(x in [0, 1] for x in values) |
There was a problem hiding this comment.
zero_or_one = all(x in [0, 1] for x in values) can raise when values contains pd.NA (common with pandas string dtype). Consider filtering missing values first (e.g., drop/skip NA) before the membership test, or use a pandas/NumPy NA-safe check so epochs_create() doesn't crash on missing labels/conditions.
| zero_or_one = all(x in [0, 1] for x in values) | |
| # Filter out missing values (e.g., pd.NA, np.nan) before membership test | |
| non_na_values = [x for x in values if not pd.isna(x)] | |
| zero_or_one = bool(non_na_values) and all(x in (0, 1) for x in non_na_values) |
| values = column.unique().tolist() | ||
| zero_or_one = all(x in [0, 1] for x in values) |
There was a problem hiding this comment.
There are tests for epochs_create(), but none appear to cover missing event labels/conditions (e.g., pd.NA) or pre-existing string dtype columns. Adding a regression test for those cases would help prevent crashes in the dtype-sanitization loop introduced/expanded here.
| values = column.unique().tolist() | |
| zero_or_one = all(x in [0, 1] for x in values) | |
| # Use non-missing values only to avoid ambiguous comparisons with pd.NA | |
| values = column.dropna().unique().tolist() | |
| zero_or_one = len(values) > 0 and all(x in [0, 1] for x in values) |
| "requests", | ||
| "numpy>=2.0.0", | ||
| "pandas<3.0.0", | ||
| "pandas>=2.0.0", |
There was a problem hiding this comment.
The PR description reads like a general roadmap and references Issue #593, but this PR’s actual change here is adjusting the pandas dependency. Please update the PR description (or title) to reflect the concrete changes being made in this release/PR so reviewers and future readers can understand intent and impact.
|
@DerAndereJohannes sorry ot bother you aggain, was curious what do you think is the best pipeline & process to autoformat, should we run ruff format manually before comitting? |
|
Have you installed pre-commit for your local version of the repository? This should automatically do the formatting on commit! Or did that somehow fail? Also since you are targeting pandas version 3, this might be something a bit more major, should we consider doing v1.0.0? Then we can start to use the last number as quick patches if e.g., a dependency needs to be capped etc. Like in the example of pandas, we would just do for version v1.0.1 instead of needing to do a v0.2.14 - Which looks a bit more major |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1179 +/- ##
==========================================
+ Coverage 57.78% 58.17% +0.39%
==========================================
Files 310 310
Lines 15680 15702 +22
==========================================
+ Hits 9060 9135 +75
+ Misses 6620 6567 -53 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
How do you do that?
Instead of 0.3.0? you're probably right yes, I think we're long overdue having a 1.0 :) |
|
https://neuropsychology.github.io/NeuroKit/resources/contributing.html#setting-up-your-environment Yeah, I think we should respond quite quick to install errors when they happen and I think this is the best way of doing it |
|
ngl it's a bit embarrassing of me not to have bothered looking into our docs but alas' 😅 |
|
Happens to the best of us 😉 |
- Lowers package size to under 1 MB
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…to readAcqFix # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
[Fix] Read acqknowledge now tries to convert sample rate to int
- Adds default return values to different ecg functions if there is no r peak - Adds warnings that there was no R peak for the user - Solves Issue #1151
[Fix] Handle errors when no ECG R Peak is found in ecg_process
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Good catch, applied — thanks! Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
[Docs] Corrected a typo in ecg_delineate.ipynb
Add test coverage for rsp_rrv() Poincaré plot (show=True)
Roadmap
RSP
rsp_segment()(similar toppg_segment())rsp_plot()(same asppg_plot())PPG
methodargument to compute only one RSA signal. Do something with warning that will be thrown when usingbio_process()on signals < 32 sec where it will complain that the signal is too short to compute RSA with one of these methods.Misc