-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Add check_time_intervals_duration to verify TimeIntervals table duration and include unit tests.
#635
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
base: dev
Are you sure you want to change the base?
Changes from all commits
4c06957
539d25e
1476eff
e0c27f9
c944bb5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| NELEMS = 200 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MAX_DURATION = 3600 * 24 * 365.25 # default: 1 year | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @register_check(importance=Importance.CRITICAL, neurodata_type=DynamicTableRegion) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -292,3 +293,61 @@ def check_table_time_columns_are_not_negative(table: DynamicTable) -> Optional[I | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @register_check(importance=Importance.CRITICAL, neurodata_type=TimeIntervals) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def check_time_intervals_duration( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| time_intervals: TimeIntervals, duration_threshold: float = MAX_DURATION | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Optional[InspectorMessage]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Check if the duration spanned by time columns in a TimeIntervals table exceeds a threshold. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| This check examines start_time, stop_time, and any other columns ending in _time. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Best Practice: :ref:`best_practice_time_interval_time_columns` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Parameters | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ---------- | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| time_intervals: TimeIntervals | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| The table to check | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| duration_threshold: float, optional | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Maximum expected duration in seconds. Default is 1 year (365.25 days). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(time_intervals.id) == 0: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| start_times = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end_times = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Check for start_time and stop_time columns | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if "start_time" in time_intervals.colnames and len(time_intervals["start_time"]) > 0: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and with the other time columns we are assuming that the time columns are already well-ordered. Is there a way of running checks in order? Maybe this check does not make sense if the other fails and the output will confuse rather than clarify if the ascending order check fails.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this check requires that the times are in order. If they are not, this check may fail to raise a message. I suppose we could just read all the data. That's probably fine in most cases -- I doubt we'll come across many datasets where these datasets are large. Do you think it's better to just read all of the time arrays?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is definitely more safe. If we want to be more efficient maybe we can just combine these two checks: nwbinspector/src/nwbinspector/checks/_tables.py Lines 52 to 80 in d8382c9
To avoid reading them twice. I think either way is fine. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| start_times.append(float(time_intervals["start_time"][0])) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if "stop_time" in time_intervals.colnames and len(time_intervals["stop_time"]) > 0: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end_times.append(float(time_intervals["stop_time"][-1])) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Check for other time columns | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for column_name in time_intervals.colnames: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that all the times in the time columns should be smaller than the |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| column_name.endswith("_time") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and column_name not in ["start_time", "stop_time"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and len(time_intervals[column_name]) > 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| data = time_intervals[column_name] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| start_times.append(float(data[0])) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end_times.append(float(data[-1])) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if start_times and end_times: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| duration = max(end_times) - min(start_times) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if duration > duration_threshold: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| duration_years = duration / 31557600.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| threshold_years = duration_threshold / 31557600.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return InspectorMessage( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| message=( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"TimeIntervals table '{time_intervals.name}' has a duration of {duration:.2f} seconds " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"({duration_years:.2f} years), which exceeds the threshold of " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"{duration_threshold:.2f} seconds ({threshold_years:.2f} years). " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Please verify that this is correct." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Should this be a critical error? We are basically saying that they won't ever be such cases. Seems too strong to me.
We should have a way of handling "things that are usually errors but there is a small probability that the user knows what they are doing and they can move forward"
The solution could a non-trivial barrier to enable this like an environment variable that could skip this error (as a CLI argument would be hard to propagate to DANDI) or something of the like.