feat(review): Session Review as a Resolve-style editor, with markers and an epoch table - #182
Merged
Merged
Conversation
… it did in a range
…ed by the project manifest
…uty-shaded pulses
…rl-scroll to zoom
…rward ones, stale drags end
…and zone sections
…dges and ethogram strips
… reads tools.qss through load_stylesheet
…inspector, full-width timeline
…ge menu, loop, HUD, hardware stats
…covery A cp1252 notes.csv beside a recording raised UnicodeDecodeError out of discover(), and a manifest whose sessions or subjects were not objects raised AttributeError past project_groups, which only catches ProjectError. Header sniffing now skips undecodable files, and every wrong manifest shape (non-UTF-8, sessions/subjects not an object, a session record not an object) is a ProjectError naming the file, so the cohort still opens ungrouped with a warning. A non-object provenance is tolerated, as a malformed treatment already was.
…ames The live tracker issues a new object_id after a detection loss, so a single-animal recording was cut off at the first id change. Ids now split into subjects only when two of them share a frame; otherwise every row with an id >= 0 is the one animal. A tracking CSV with negative frame numbers raised ValueError or IndexError out of from_recording, or silently wrapped positions to the far end of the array. It is now a SessionViewError with the reason.
…ifier lane Zoomed out, each viewport change cost ~60 ms per whole-session pulse train: a fillRect and an x_of_axis call per segment. Switch lanes now place segments with one vectorised _xs call, draw spans >= 1 px individually, a lone sub-pixel pulse solid at 1 px, and columns holding more than one switch at their exact ON coverage. 30,000 segments at 1400 px repaint in ~6 ms. Lane.binary (plus ends_ms and values) is cached on the lane; is_binary() stays as a wrapper. Also: the navigator strip is built at the device pixel ratio, the header menu is deleted after use, and a view whose labels are all blank no longer synthesises an empty lane titled Classifier.
Viewport.follow made the playhead the left edge in both directions, so reverse playback paged on every tick; before the window it now becomes the right edge. Pool rows put the badges on their own line and elide the name, so VID/POSE/HW are whole at the default width. The blank canvas points at the inspector's Session tab, and a recording is not told to choose a pose CSV. The tour's ethogram step uses an em dash.
…w fixes GLIDER installs no excepthook, so an exception out of a menu action ends the process. Opening a recording, a cohort folder or an ethogram now catches (SessionViewError, OSError, ValueError, KeyError), and a cohort folder that cannot be walked is reported. A folder with events but no tracking CSV falls through to its single ethogram_raw.csv, and only if that also fails reports both. The parsed-recording cache is cleared on every open, so a recording opened mid-run is read again. Also: the Cohort table fills only while its tab is shown (a drag recomputed the whole cohort per mouse move); playback at 8x lands on the last frame; Ctrl/Cmd edit keys and auto-repeated J/L are ignored; the range menu is deleted after use; _set_cohort builds before it assigns; a recording's centroid reads as Centroid, not poses; the alignment warning counts from the first video frame; Select All shows the platform's own shortcut text.
…lanes, larger behaviour HUD The transport and bout steppers drew text glyphs (⏮ ◀▏ ▶ ▕▶ ⏭ ◀ ▶) that read badly; they now use Lucide icons (ISC, vendored beside the existing set) through a small lucide_icon() helper that tints currentColor and renders at 1x and 2x. Play shows Pause while playing, icon and word together. The navigator's whole-session ethogram grows from an 18 px hairline to a 34 px band (navigator 30 -> 48 px), behaviour lanes from 30 to 44 px, and the HUD's behaviour chip -- the one label read while scrubbing -- is set large and bold.
… size The transport's bout read-out was a plain QLabel, so its text width was part of the row's minimum width -- which is the viewer panel's. Each new bout pushed the splitter and the video slid sideways (908 -> 1062 px on a long behaviour name). The read-out is now an ElidedLabel (moved from the sessions panel into tool_ui for both to share) that takes the space left over, and the clock and frame counter are fixed to their widest text. The HUD's behaviour chip is sized once per session, to the widest behaviour with a long seconds-in, so its edge no longer twitches with every digit. A long behaviour at the playhead in a lane header elides clear of the lane's own title instead of running over it. The top-bar menus shared no size: Open took the primary role's larger padding, and every menu arrow sat in its button's bottom-right corner. They now share one padding and width, with the arrow centred in room of its own; the ? menu matches their height and drops the arrow.
…frame 0 ExactFrameReader reached any earlier frame by rewinding to 0 and decoding forward, because a seek on long-GOP video lands several frames off and cannot say where. In Session Review that made every backward step, click, J, loop, and turning the video back on after scrubbing cost a decode of the whole session up to the playhead: one step back at frame 40,000 decoded 40,000. A decoded frame carries its own timestamp, which says where a seek really landed. The reader now seeks just before its target, reads the landing from that timestamp, and walks the rest; a short hop ahead still just walks, and reading the frame already held (the zone editor's grab) decodes nothing. Every read checks the timestamp against the frame asked for: a file whose timestamps do not count its frames -- variable rate, none at all -- is caught on the first disagreement and read by counting from 0 as before, and a frame reached through a seek that failed the check is fetched again that way. A backward jump now costs under 40 decodes on a mis-seeking capture, and the pixels match the old reader's frame for frame on a real mp4 and on long-GOP mp4v and avc1 clips numbered frame by frame.
Checked against the lab's Blast Pilot cohort: on a whole 18,705-frame camera file (15.6 fps, capture-time stamps with +-4 ms of jitter) the timestamps named every frame, and seeks landed exactly on 30 fps files too. The one gap left -- a frame dropped where the reader never walked -- is written down in the docstring with its upgrade path.
Phase 1 applied the shown session's frame window to every session, which is a different stretch of protocol in any animal whose rig ran longer before flow start or recorded at another rate. The range now goes through the time rule, switching animals keeps it in seconds, and each row carries the frames and seconds it was measured over, plus time on per device.
…d on the navigator
…tor, and their files
… than reading empty
…are gone SessionView.load hardcoded 30 fps whenever a session's pose CSV could not be found (offline video, network share, a copy without the DLC sidecar), silently mislabelling every frame of a session recorded at any other rate. run.json already records the fps a run classified at; fall back to it, the same way applied thresholds and px_per_mm already do. A reachable pose CSV still outranks it, unchanged.
… stale ranges Three findings from the Phase 2 branch review, fixed together: - save_markers no longer strips a shared marker file down to 0600. mkstemp creates its temp file owner-only, and os.replace carried that mode onto the destination, so saving over a group-readable review_markers.json left the next lab member who opened it hitting PermissionError. The temp file now matches an existing destination's mode, or a fresh file's umask-masked 0666. - The epoch table zero-fills a behaviour across every epoch, not just the one being rendered. range_rows only zero-filled `<state>_s` over the states seen in its own call, but the catalog is the union across all epochs, so a behaviour seen only in one range left every other range's column reading "-" where 0.00 was the true measurement, and the tidy/wide exports carried NaN instead of 0.0. - Loading a new cohort now clears `_current_span`. It used to survive a cohort switch and get re-applied by `_show_session(0)` as a time range over an unrelated experiment, adding a bogus "Current range" epoch column measured over a span chosen elsewhere. Also documents the fps-from-manifest fix (0a4545b) in the CHANGELOG, found against the Blast Pilot cohort's 15.59 fps sessions.
…'s own font This morning's fix gave them a stylesheet min-width of 64 px, which made them one size only while every label fit inside it: true of the macOS font, not of Windows', where CI measured Open, Zones and Export at 110, 118 and 130 px. The window now measures the three after the theme is applied and gives each the widest one's width, so the platform's font decides the size and the buttons still agree.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Session Review has been a stack of cards: a header of seven buttons, a canvas, a 46 px bar and a tab of tables. It had no zoom (one pixel of a 45,000-frame session is tens of frames), and it could only open an ethogram CSV. A live rig recording with video, tracking and hardware events but no classifier run could not be opened at all.
This PR rebuilds it as an editor modelled on DaVinci Resolve's Edit page. Each track is a behaviour source or a hardware device instead of an audio or video track. You scrub the video, drag a range, and read what happened in it. It carries phases 1 and 2:
Hardware-driven selection ("select all ON periods") is phase 3, and is not here.
What you get
Layout (chosen from browser mockups):
Timeline:
ON,ON · 10 Hz,90°, the current behaviour.HH:MM:SS:FF, flow-relative) with the pre-flow period hatched.Mouse and keys:
Viewer:
Inspector:
20.0 s on · 200 pulses,1× at 2:25.0,90° → 180° at 2:30.0) and Zones.Opening:
groupinglider_project.json.Phase 2: markers and the epoch table
Markers:
Epoch table (replaces the Cohort tab beside the timeline):
—with a tooltip saying why where an animal can't answer (no calibration, no zones, range outside the session).Behaviour changes (also in CHANGELOG)
start_s,end_sandt0.Design decisions
session_view.pyalready set.analysis/timeline.pygainsvalue_at,rate_at,hardware_in_rangeanddescribe_*.analysis/cohort.py(new) handles discovery and session ids.analysis/markers.py(new) holds the time rule, theMarkermodel, and the marker files.analysis/epochs.py(new) holds the metric catalog, mean and SEM, and the tidy and wide frames.gui/review/viewport.pyholds the zoom, pan, snap and timecode arithmetic.gui/review/package.analysis_window.pyis now the shell that wires them.selection_changedstill emits frames.segment_statsandzone_rowsare untouched.seconds_at/frame_at/frames_indo every conversion — markers, snapping, the current range, cohort ranges and exports — so they cannot disagree about where a second falls. The current range is held in seconds: set when you select, never re-derived from the animal on screen, so a 15.59 fps animal (which cannot show 1.033 s exactly) cannot quietly move it.review_markers.jsonsits in the session's folder andcohort_markers.jsonin the folder opened as a cohort — not inglider_project.json(which would couple to its schema and need a manifest), and not in QSettings (not portable). Writes are atomic (temp file, thenos.replace) and keep the file's existing permissions, so a cohort file on a shared lab drive stays readable to the rest of the lab. A file that cannot be read, or that a newer GLIDER wrote, is never overwritten: its markers turn read-only, the tools say why, and the error is shown once.SessionView(SessionView.from_recording), so the canvas, trail, heatmap, zones and range statistics work unchanged. Three details decided this:first_video_framemakes the viewer read video frame n − 1; without it every frame shows its neighbour.# Pixels/mmheader, scaled to the viewing resolution. It is never taken asdistance_px / distance_mm: an uncalibrated logger writes the same number into both, which would claim 1.00 px/mm.elapsed_msspan.Session.frame_ratetakes the median of millisecond-rounded timestamps and reads 30.30 for a 30 fps rig, which would skew every per-second number by 1 %.object_idafter a detection loss. A recording is split into subjects only when two ids ≥ 0 share a frame; otherwise one animal would be silently truncated at the first id change.glider_project.jsonunder a cohort folder used to raise out of a menu action and kill the process. Both are handled at the shared roots (_io._classify_csv,Project.load), and a bad manifest only costs the grouping, with a warning.colors.py. A test checks that every hex in the Session Review stylesheet matches one. Markers use nine named swatches, saved by name, so retuning a colour recolours old markers too.Fixes found along the way
ExactFrameReaderreached any earlier frame by decoding forward from frame 0, so one step back at frame 40,000 decoded 40,000 frames, and turning the video back on after scrubbing walked from wherever it had stopped. It now seeks just before its target and reads the landed frame's own timestamp to know exactly where it is, falling back to counting from 0 the moment a file's timestamps disagree with its frames. Frames are identical to the old reader's. On a 20-minute lab camera recording, a step back at frame 14,025 took 23 ms, where the old reader decoded 14,025 frames to get there.SessionView.loadfell back to 30 fps whenever the pose CSV could not be found, so a 15.59 fps recording read every duration and speed at nearly twice its rate. It now reads the rate from the run manifest; a reachable pose CSV still wins.Tests
5389 → 5700 passed (4 skipped, 5 deselected);
ruffandblackclean. CI green on Windows, Ubuntu and macOS.tests/unit/gui/review/(timeline, navigator, viewer, inspector, pool, marker editor, epoch table),tests/unit/analysis/test_cohort.py,test_markers.py,test_epochs.py, and the recording, keys, menu, loop, HUD, hardware-stats, time-rule, marker and epoch-table classes intest_analysis_window.py.test_timeline_bar.pywas retired, and its cases live intest_timeline_view.py. The Cohort-tab tests became epoch-table tests.tests/integration/test_review_real_recording.py(markerreal_recording) is skipped unlessGLIDER_REVIEW_RECORDINGpoints at a rig recording folder. It checks that real tracking frames line up with the real video, and that a real rig's events become lanes. It has not been run against a rig recording yet.Each task in both phases (13 in phase 1, 12 in phase 2) was reviewed for spec and quality, and so was each whole branch; findings were fixed in-branch.
Known follow-ups (not in this PR)
build_timelinestill draws one "Live state" lane per tracker id, so a recording whose id changed shows several lanes while the statistics treat it as one animal.Session.frame_ratereads 30.30 fps for a 30 fps rig, as above.from_recordingworks around it; other callers still see it.Not in this PR