-
Notifications
You must be signed in to change notification settings - Fork 1
TST: fix remaining tests #497
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
Conversation
Reviewer's GuideAligns index/timedelta handling and tests with pandas>=3.0 behavior by enforcing explicit index dtypes, normalizing timedelta units to ns, and updating misc/table utilities and tests to reflect the new typing semantics. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- In
audformat/core/testing.py::add_table,to_timedeltais now referenced directly but is not imported into the module; either import it (e.g. fromaudformat.core.index) or fully qualify the call to avoid aNameError. - The tests now mix
dtype="string",dtype="str", anddtype="object"for similar string-like indices/series; consider standardizing on one representation where possible to avoid subtle differences in behavior across pandas versions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `audformat/core/testing.py::add_table`, `to_timedelta` is now referenced directly but is not imported into the module; either import it (e.g. from `audformat.core.index`) or fully qualify the call to avoid a `NameError`.
- The tests now mix `dtype="string"`, `dtype="str"`, and `dtype="object"` for similar string-like indices/series; consider standardizing on one representation where possible to avoid subtle differences in behavior across pandas versions.
## Individual Comments
### Comment 1
<location> `audformat/core/index.py:29-31` </location>
<code_context>
r"""Convert time value to pd.Timedelta."""
try:
- return pd.to_timedelta(times, unit="s")
+ return pd.to_timedelta(times, unit="s").as_unit("ns")
except ValueError: # catches values like '1s'
- return pd.to_timedelta(times)
+ return pd.to_timedelta(times).as_unit("ns")
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `.as_unit("ns")` assumes the return value always supports this method, which can break for Series inputs.
`pd.to_timedelta` may return a `Series`, `TimedeltaIndex`, scalar `Timedelta`, or ndarray. Only some of these support `.as_unit`, so passing a `Series` (e.g. a `pd.Series` of time strings) will now raise `AttributeError` where it previously worked.
To keep behavior consistent, consider normalizing via dtype instead, e.g. `result = pd.to_timedelta(...); result = result.astype('timedelta64[ns]')` for array-likes, and only calling `.as_unit('ns')` on scalar `Timedelta` values (or branching on `isinstance(result, pd.Timedelta)` / `Index` / `Series`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
audformat.core.index.to_timedelta()returnstimedelta64[ns]formataudformat.segmented_index()audformat.testing.add_table()to preserve ns resolution whenfile_durationis given as a stringtests/test_misc_table.pyobject/string handlingtests/test_table.pytimedelta handlingtests/test_utils.pyobject/string handlingtests/test_utils_concat.pyobject/string handling