Conversation
Refactored the Epochs class to use a dictionary of epochs indexed by epoch index, added properties for empty epochs, and improved overlap checking and warnings. Updated the to_numpy method to support flexible sampling rates and interpolation, and improved baseline correction error handling. Modified plot_epochs and its helpers to work with the new Epochs API, and updated the pupil_size_and_epoching tutorial to use the new sample data and API.
Expanded and clarified docstrings for epoching functions and the Epochs class, including detailed parameter and return value descriptions. Refactored Dataset to automatically handle native data without requiring a 'custom' flag, and improved section construction for native recordings. Updated tutorials to use consistent event naming and native data examples. Minor bugfixes and doc improvements in export and utility modules.
|
@JGHartel Currently the baseline correction method is not updated yet. What do you think would be the best API/code for it? For operability with other methods such as |
JGHartel
left a comment
There was a problem hiding this comment.
Overall, I welcome the suggested changes. In particular, the rename towards epoch_info and the unified access to epochs data as epoch.epochs, rather than the previous mixed access, is appreciated. Still, I would like to invite some discussions on the assumed datatypes, e.g. epochs.epochs and epochs.annotate being a dictionary. This would especially futureproof the development, if dataset level operations should be introduced
pyneon/epochs.py
Outdated
| # Create epochs | ||
| self.epochs, self.data = _create_epochs(source, times_df) | ||
| @cached_property | ||
| def epochs(self) -> dict[int, Stream | Events | None]: |
There was a problem hiding this comment.
flagging this in case we ever want to support multi-recording functionality. It would be good to have a Unique ID for each event, so that one can easily concat these dicts between recordings. Alternatively, we could then construct an implicit multiindex (recording, epoch_number), which would require a custom concat function.
There was a problem hiding this comment.
This seems a bit of an advanced user case and also in principle should apply to other classes (e.g. streams and events). My evaluation would be it would require another PR if we see value in supporting multi-recording concatenation
There was a problem hiding this comment.
Pull request overview
This PR introduces a major refactoring of the Epochs class to make it more intuitive and easier to use. The key changes include moving away from nested DataFrames to a dictionary-based structure, renaming methods for clarity, and introducing lazy computation of epochs using cached properties.
Changes:
- Refactored
Epochsclass to useepochs_dict(cached property returning a dictionary) instead of nested DataFrames - Renamed
event_timestoepochs_infoandevents_to_times_dftoevents_to_epochs_infofor better comprehension - Changed
Eventsclass to use event IDs as DataFrame index instead of separate columns - Added new
filter_by_namemethod to Events class and updatedfilter_by_durationbehavior - Added PLR sample dataset support with Figshare hosting
- Consolidated CI workflows (removed separate tests.yml, integrated into main.yml)
Reviewed changes
Copilot reviewed 19 out of 22 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| pyneon/epochs.py | Major refactoring of Epochs class with new dictionary-based structure, cached properties, and helper functions renamed |
| pyneon/events.py | Modified to use event IDs as index, added filter_by_name method, updated filter_by_duration |
| pyneon/stream.py | Added stub for detect_events_from_derivative method, updated docstrings with reusable doc snippets |
| pyneon/vis/vis.py | Updated plot_epochs to work with new epochs_dict structure |
| pyneon/dataset.py | Made sections.csv optional, improved error handling for missing recordings |
| pyneon/utils/doc_decorators.py | Added reusable documentation snippets for common return types and parameters |
| pyneon/utils/sample_data.py | Added PLR dataset URL and test |
| tests/conftest.py | Added simple_events fixture for testing Events functionality |
| tests/test_events.py | Added (empty) test stub for crop functionality |
| tests/test_streams.py | Added tests for interpolation and concatenation methods |
| .github/workflows/main.yml | Consolidated CI workflows, added test job dependencies |
| source/tutorials/*.ipynb | Updated tutorial notebooks to use new API names |
| README.md | Added information about sample datasets on Figshare |
Comments suppressed due to low confidence (1)
pyneon/epochs.py:223
- The properties
columnsanddtypes(lines 216-223) referenceself.datawhich no longer exists in the newEpochsclass structure. These properties need to be updated or removed.
@property
def columns(self) -> pd.Index:
return self.data.columns[:-3]
@property
def dtypes(self) -> pd.Series:
"""The data types of the epoched data."""
return self.data.dtypes[:-3]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Remove legacy helper and unused imports from utils, update exports, and tighten docs; specifically remove load_or_compute and its export, drop unused Path/Callable imports, and keep only runtime utilities. Improve video module/docs and API: add a module docstring, expand Video class documentation, add a cap property (exposes underlying cv2.VideoCapture) and richly document it and VFR warnings, reintroduce/expand delegate methods (isOpened, grab, retrieve, read, set, get) with types and guidance, clarify reset/release/close behavior, remove der_dir attribute, and strengthen read_frame_at and timestamp_to_frame_index docstrings. Also apply small docstring fixes in Dataset and Epochs to use local class/reference names consistently.
Rename and standardize detection format, update homography API, and refresh docs. - Rename DETECTION_COLUMNS -> SURFACE_CORNER_DETECTION_COLUMNS and update all callers (marker, surface, vis, marker verification). - Change detect_surface output to use "surface id" and set timestamp index earlier; raise on empty detections instead of returning empty DataFrame. Improve error messages and validation. - Simplify and refactor find_homographies: rename parameters to detections/layout, auto-detect marker vs surface modes, validate layouts, compute marker corner reference points from marker layout (size/center), and remove several helper functions. Improve input validation and error messages. Return homographies as a Stream as before. - Update detect_markers to verify against the renamed constant. - Update docstrings for Motion-BIDS and Eye-Tracking-BIDS exports with references/links and clearer descriptions. - Adjust doc_decorators formatting (string interpolation and DOC entries) and remove/streamline obsolete doc blocks. - Minor fixes: whitespace cleanup in video.Video docstring, add Sphinx suppress_warnings for autosummary stub files in source/conf.py. - Update tutorial notebook to use new find_homographies parameter name (layout=...) and clear execution counts; bump notebook metadata version. These changes unify detection column naming, make the homography function easier to use with either marker or surface detections, and improve documentation and validation for downstream users.
This reverts commit c91aad4.
Rename the eye export function for clearer naming. The function definition in pyneon/export/export_bids.py was changed from export_eye_bids to export_eye_tracking_bids; the export package re-export and __all__ in pyneon/export/__init__.py were updated accordingly; and imports/usages in pyneon/recording.py (the Recording method name and its internal call) were updated to match. No functional behavior was changed.
Use explicit quaternion component names (quat_w/quat_x/quat_y/quat_z) and rename pupil fields to left_pupil_diameter/right_pupil_diameter in BIDS metadata. Update export logic to remove pupil diameter entries when no eye_states are present. Adjust example notebook to call the new export_eye_tracking_bids API, clear execution count, and bump notebook kernel version.
pyneon/video/surface.py
Outdated
| if df.empty: | ||
| print("Warning: No surface contours detected.") | ||
| cols = list(DETECTION_COLUMNS) | ||
| if report_diagnostics: | ||
| cols.extend(["area_ratio", "score"]) | ||
| df = pd.DataFrame(columns=cols) |
There was a problem hiding this comment.
Stream will return error when df is empty. Raise error here instead
pyneon/video/homography.py
Outdated
| surface_layout : pandas.DataFrame or numpy.ndarray, optional | ||
| Surface layout with a ``corners`` column or a 4x2 numpy array. Required | ||
| for surface-corner detections. |
There was a problem hiding this comment.
what is the order of the 4 corners?
pyneon/video/homography.py
Outdated
| actual_cols.add(detection_df.index.name) | ||
|
|
||
| uses_marker_columns = set(DETECTION_COLUMNS).issubset(actual_cols) | ||
| uses_corner_column = "corners" in detection_df.columns |
There was a problem hiding this comment.
Corners should be retired completely
pyneon/video/homography.py
Outdated
| if not isinstance(marker_layout, pd.DataFrame): | ||
| raise ValueError("marker_layout must be a DataFrame for marker detections.") | ||
| if not set(MARKERS_LAYOUT_COLUMNS).issubset(marker_layout.columns): | ||
| raise ValueError( | ||
| "marker_layout must contain the following columns: " | ||
| f"{', '.join(MARKERS_LAYOUT_COLUMNS)}" | ||
| ) |
pyneon/video/homography.py
Outdated
| detected_markers: Stream | pd.DataFrame, | ||
| marker_layout: Optional[pd.DataFrame] = None, | ||
| surface_layout: Optional[pd.DataFrame | np.ndarray] = None, | ||
| valid_markers: int = 2, |
There was a problem hiding this comment.
I think valid markers can be defaulted to 2 and it only applies to marker detections. Is it correct that surface detections always have 4 points? So the argument was never relevant?
pyneon/video/homography.py
Outdated
| return marker_layout | ||
|
|
||
|
|
||
| def _prepare_corner_layout( |
There was a problem hiding this comment.
this helper function is really hard for me to understand. First, the name seems to suggest it parses corners (backward compatible?), but it's executed when surface layout is supplied. The output tuples lack sufficient explanation to make sense of, especially when compared to the parsing of marker layout
Enhance Motion- and Eye-Tracking-BIDS export behavior and metadata. Import nominal_sampling_rates and update MOTION_META_DEFAULT to set sensible defaults and explicit channel counts; rename tracking system to "Neon IMU". Add _infer_prefix_from_dir to infer sub-/ses- prefixes from directory layout and use that when prefix is not provided; validate required prefix fields and extract task name for metadata. Populate channels.tsv with recommended/optional fields (placement, sampling_frequency, status, status_description) and compute channel counts for the motion JSON. Write gzipped physio/physioevents files using gzip, guard for pupil columns, and include task name in eye metadata. Update scans handling to use the inferred sub_ses prefix and avoid duplicate entries. Update docstrings, README/notebook examples, and adjust tests to match new TrackingSystemName and channel columns.
When mode picks a single candidate, selected is now a dict instead of a one-element list and the loop is removed. The code builds one detection_row (surface id set to 0) directly, and diagnostic fields now read from selected rather than iterating. This simplifies the flow and avoids creating a temporary list; note that callers expecting multiple surface rows per frame may be affected.
Introduce DataFrame validation helpers and refactor video-related utilities and docs. Added _validate_neon_tabular_data and _validate_df_columns (and exported them) to centralize DataFrame checks, replacing prior _check_data calls across preprocess, stream, and events. Renamed video constants module to variables and expanded marker/surface column definitions; added validations for marker/layout and surface layouts. Reworked homography computation to handle marker vs surface inputs, simplified detection flows, and unified returned Stream shapes. Enhanced doc decorator system (nested placeholder filling and renamed doc snippets), added typing and attributes to Stream/Events, introduced Video.timestamps/ts properties, and applied various formatting and I/O fixes (gzip usage and whitespace).
Rename and refactor surface-detection API to use "contour" terminology and adjust related internals. surface.py was renamed to detect_contour.py and detect_surface -> detect_contour; docs keys updated (detect_contour_params/returns). Detection column constants were refactored (DETECTION_COLUMNS -> DETECTION_COLUMNS_BASE, SURFACE_DETECTION_COLUMNS -> CONTOUR_DETECTION_COLUMNS) and validation helpers renamed (_validate_surface_layout -> _validate_contour_layout). Homography, video package exports, Video.detect_* method, and visualization code were updated to use the new names and to validate against the base detection columns. Minor changes: sample_data wording updated to mention fiducial markers, plotting loop variable renamed for clarity, and imshow origin set to 'upper'. The surface mapping tutorial notebook was also updated accordingly.
Epochs
Developers and users have found the old
Epochsclass quite unintuitive and operationally hard to use (e.g., nested DataFrame). This PR aims to reorganize the class.Current features:
event_timesis renamed toepochs_infofor easier comprehensionEpochsinstance will not compute the epochs or annotated data automatically.epochsis now a cached property in the form of a dictionary. Keys are indices ofepochs_infoand values areStream/Eventsinstances.annotate_epochs.Marker mapping
Per suggestions, this PR also aims to expand the marker mapping from AprilTags to also ArUco markers (using OpenCV's aruco module). This direct interface also allows us to get rid of the optional dependency of
pupil-apriltags. API and documentations are improved along the way.