-
Notifications
You must be signed in to change notification settings - Fork 163
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
[ENH] events.tsv should be sorted by onset values #1732
Conversation
Remi-Gau
commented
Mar 20, 2024
- closes Event onset times can be out of order #1691
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1732 +/- ##
=======================================
Coverage 87.93% 87.93%
=======================================
Files 16 16
Lines 1351 1351
=======================================
Hits 1188 1188
Misses 163 163 ☔ View full report in Codecov by Sentry. |
Will propose schema rule. Bug me if I don't do this soon enough for you. |
src/schema/rules/checks/events.yaml
Outdated
selectors: | ||
- suffix == "events" | ||
- extension == ".tsv" | ||
checks: | ||
- sorted(columns.onset) == columns.onset |
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.
@rwblair Do you agree?
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.
n/a would be the only thing to mess it up if allowed. Otherwise looks good.
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.
Good point. I think it being a warning should make that an annoyance, not a show-stopper. Will add a comment, though.