fix: compute Experiment start/end time as union across all devices - #137
Conversation
|
Review these changes at https://app.gitnotebooks.com/sensorium-competition/experanto/pull/137 |
There was a problem hiding this comment.
Pull request overview
Fixes Experiment.start_time / end_time so they reflect the global time span across all loaded devices (earliest start + latest end), matching the class docstring and avoiding “last device wins” behavior.
Changes:
- Update
Experiment._load_devices()to computestart_time/end_timeviamin/maxacross devices. - Add new tests covering 1-, 2-, and 3-device scenarios for the computed experiment time span.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
experanto/experiment.py |
Compute experiment start/end time as min/max across loaded devices. |
tests/test_experiment.py |
New regression tests for experiment start/end time spanning multiple devices. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
pollytur
left a comment
There was a problem hiding this comment.
We typically separate the test data creation and the actual testing - please do the same here :)
Ideally look at PR #113 and name the files like create_experiment.py and test_experiment.py such that we could merge your tests together later
|
Thanks for the feedback! I'll restructure the tests following the pattern in PR #113 — separating the data creation helpers into create_experiment.py and keeping the tests in test_experiment.py. Will push the update shortly. |
There was a problem hiding this comment.
Pull request overview
Fixes Experiment.start_time/end_time so they reflect the global time span across all loaded devices (earliest start + latest end), instead of only the last device processed.
Changes:
- Update
Experiment._load_devices()to trackstart_timeviamin(...)andend_timeviamax(...)across devices. - Add new tests covering single-, two-, and three-device scenarios for the computed union time range.
- Add a small test helper to generate multi-device experiment folders under
tmp_path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
experanto/experiment.py |
Computes experiment-wide start/end as min/max across device ranges. |
tests/test_experiment.py |
Adds regression tests validating union start/end time behavior. |
tests/create_experiment.py |
Adds helpers to build multi-device experiment fixtures for tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Fixes Experiment.start_time / end_time so they reflect the union of time ranges across all loaded devices (earliest start, latest end), aligning behavior with the class docstring and preventing “last device wins” behavior.
Changes:
- Update
Experiment._load_devices()to track experiment-widestart_time/end_timeviamin/maxacross devices. - Add warnings/validation when devices have undefined time bounds (though current placement/logic needs adjustment).
- Add unit tests covering single-, two-, and three-device unions using new test fixtures.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
experanto/experiment.py |
Compute experiment time range as min/max across devices; adds (currently flawed) validation/warnings for missing/invalid ranges. |
tests/test_experiment.py |
Adds tests asserting Experiment.start_time/end_time represent the union across devices. |
tests/create_experiment.py |
Adds helpers to generate minimal on-disk experiment/device fixtures for the new tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes Experiment.start_time / end_time so they reflect the global time span across all loaded devices (earliest start + latest end), rather than being overwritten by the last device processed.
Changes:
- Update
Experiment._load_devices()to compute experiment-widestart_time/end_timeviamin/maxacross devices (skipping devices with undefined bounds). - Add a small post-load validation/warning block for missing devices / invalid computed range.
- Add new tests covering single-, two-, and three-device union behavior (with new test fixtures for building a minimal experiment folder).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| experanto/experiment.py | Compute experiment-wide time range across devices and add validation/warnings. |
| tests/test_experiment.py | Add regression tests for experiment-wide start/end time union behavior. |
| tests/create_experiment.py | Add helpers to create minimal on-disk device folders + modality config for tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Addressed the changes abovementioned and expanded test coverage with additional edge cases, including parametrized invalid metadata tests, a dedicated inverted range test, and |
|
@binary69 could we please merge main into the current branch? |
… skip test and add caplog assertion
8408995 to
863e52c
Compare
|
Thanks @pollytur :) will be updating soon |
|
Made the suggested changes, happy to get more feedback :) |
pollytur
left a comment
There was a problem hiding this comment.
LGTM, thanks!
@schewskone please take a look as well :)
schewskone
left a comment
There was a problem hiding this comment.
LGTM as well. I'm not 100% sure about randomness in parameter generalization for testing but I also don't mind.
Thanks for the contribution
|
Thank you so much, and I really appreciate the feedback. On the randomness concern, we can either use seeded randomness or fixed non-default valued to avoid potential flakiness. Looking forward to contribute more :) |
Closes #134
Problem
In
_load_devices(),start_timeandend_timewere assigned with plain assignment inside a loop, so they reflected only the last loaded device instead of the global range across all devices.Fix
changes
experiment.pyUpdated
_load_devices()to usemin/maxto track the earliest start and latest end across all devices.Devices with
None,NaN,inf,-inftimestamps are skipped with warning, not silently corruptingValueErrorif no device with valid time ranges are loadedValueErrorif computestart_time > end_timeNew Tests
create_experiment.pycontextmanager fixture with configurable signals, sampling rate, and
override_metafor invalid metadata testingbuilds per-device interpolation config dicts
test_experiment.pyparametrized across 1, 2, and 3 device scenarios and signal counts - (5+1)*2 = 12 runs
parametrized across 9 invalid metadata cases - 9 runs
Verifies that
start_time > end_timeraisesValueError- 1 runverifies that an invalid device is excluded and a warning is logged, while a co-loaded valid device initialises the experiment correctly. - 9 runs
All 31 tests are passing.