-
Notifications
You must be signed in to change notification settings - Fork 1
Require timedelta64[ns] in assert_index() #494
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 guide (collapsed on small PRs)Reviewer's GuideTightens Flow diagram for stricter timedelta64_ns validation in assert_indexflowchart TD
A[Call assert_index with index obj] --> B{Has level file, start, end?}
B -- No --> Z[Raise ValueError: missing required levels]
B -- Yes --> C{Is dtype of level file string?}
C -- No --> D[Raise ValueError: Level file must contain values of type string]
C -- Yes --> E{Is dtype of level start timedelta64_ns?}
E -- No --> F[Raise ValueError: Level start must contain values of type timedelta64_ns]
E -- Yes --> G{Is dtype of level end timedelta64_ns?}
G -- No --> H[Raise ValueError: Level end must contain values of type timedelta64_ns]
G -- Yes --> I[Index conforms to audformat]
File-Level Changes
Possibly linked issues
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 left some high level feedback:
- The new failing test only covers the case where both
startandendaretimedelta64[s]; consider adding a mixed-unit case (e.g.,startastimedelta64[ns]andendastimedelta64[s]) to ensure the validation logic rejects partially valid indices as well. - Instead of chaining
pd.to_timedelta(..., unit="s").astype("timedelta64[ns]")in multiple places, consider using a small helper or a directTimedeltaIndexconstructor to reduce repetition and make the intended dtype conversion clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new failing test only covers the case where both `start` and `end` are `timedelta64[s]`; consider adding a mixed-unit case (e.g., `start` as `timedelta64[ns]` and `end` as `timedelta64[s]`) to ensure the validation logic rejects partially valid indices as well.
- Instead of chaining `pd.to_timedelta(..., unit="s").astype("timedelta64[ns]")` in multiple places, consider using a small helper or a direct `TimedeltaIndex` constructor to reduce repetition and make the intended dtype conversion clearer.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:
|
* Require timedelta64[ns] in assert_index() * Add tests for mixed cases
* pandas 3.0: segmented_index() and set_index_dtypes() (#490) * Add failing test * Make test pandas 3.0.0 compatible * Fix set_index_dtypes() for pandas 3.0 * Add comment * Fix doctests * Update segmented_index() * Use segmented_index in test * Add test for segmented_index * Avoid warning in testing.add_table() (#491) * pandas 3.0: fix utils.hash() (#492) * pandas 3.0: fix utils.hash() * Fix comment * Remove unneeded code * Add more tests * Preserve ordered setting * Update comment * Fix categorical dtype with Database.get() (#493) * Fix categorical dtype with Database.get() * Update tests * Add additional test * Improve code * Clean up comment * We converted to categorical data * Simplify test * Simplify string test * Require timedelta64[ns] in assert_index() (#494) * Require timedelta64[ns] in assert_index() * Add tests for mixed cases * pandas 3.0: fix doctests output
As we require
startandendvalues of a segmented index to be of typetimedelta64[ns]we updateaudformat.assert_index()to not allowtimedelta[s].Summary by Sourcery
Require segmented index start and end levels to use nanosecond-resolution timedeltas and adjust tests accordingly.
Enhancements:
Tests: