Fix/experiment time range - #136
Closed
binary69 wants to merge 6 commits into
Closed
Conversation
|
Found 3 changed notebooks. Review the changes at https://app.gitnotebooks.com/sensorium-competition/experanto/pull/136 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to fix Experiment.start_time / Experiment.end_time so they reflect the global time range across all loaded devices (per the Experiment docstring), and adds regression tests for multi-device experiments.
Changes:
- Add new
tests/test_experiment.pycovering single-, two-, and three-device time-range behavior. - Introduce a Ruff GitHub Actions workflow and add
gsoc_env/to.gitignore. - Clean up unused imports / minor string formatting across several modules and example notebooks.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_experiment.py |
Adds regression tests asserting experiment start/end reflect the global device range. |
experanto/experiment.py |
Import cleanup only in the provided diff (core _load_devices() behavior not updated in current code). |
experanto/utils.py |
Removes unused imports and unused locals in add_behavior_as_channels. |
experanto/intervals.py |
Minor string formatting cleanup. |
experanto/interpolators.py |
Removes unused import and simplifies error message strings. |
experanto/datasets.py |
Import/unused-variable cleanup and removes a redundant override of DEFAULT_MODALITY_CONFIG. |
experanto/dataloaders.py |
Import/unused-variable cleanup. |
experanto/configs.py |
Import cleanup. |
examples/sensorium/*.ipynb |
Removes unused imports in notebooks. |
.gitignore |
Ignores gsoc_env/. |
.github/workflows/ruff.yml |
Adds Ruff lint job (currently gated by an if condition). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+117
to
+128
| experiment = Experiment( | ||
| root_folder=experiment_path, | ||
| modality_config=get_two_device_config(), | ||
| ) | ||
|
|
||
| # Union: start = min(1.0, 0.0) = 0.0, end = max(8.0, 10.0) = 10.0 | ||
| assert experiment.start_time == pytest.approx( | ||
| 0.0 | ||
| ), f"Expected start_time=0.0, got {experiment.start_time}" | ||
| assert experiment.end_time == pytest.approx( | ||
| 10.0 | ||
| ), f"Expected end_time=10.0, got {experiment.end_time}" |
Comment on lines
+11
to
+56
| EXPERIMENT_ROOT = Path("tests/experiment_data") | ||
|
|
||
|
|
||
| @contextmanager | ||
| def create_two_device_experiment( | ||
| device0_start=0.0, | ||
| device0_end=10.0, | ||
| device1_start=1.0, | ||
| device1_end=8.0, | ||
| sampling_rate=10.0, | ||
| ): | ||
| """Create a temporary experiment with two sequence devices with different time ranges.""" | ||
| try: | ||
| for device_name, start, end in [ | ||
| ("device_0", device0_start, device0_end), | ||
| ("device_1", device1_start, device1_end), | ||
| ]: | ||
| device_root = EXPERIMENT_ROOT / device_name | ||
| (device_root / "meta").mkdir(parents=True, exist_ok=True) | ||
|
|
||
| n_samples = int((end - start) * sampling_rate) + 1 | ||
| timestamps = np.linspace(start, end, n_samples) | ||
| data = np.random.rand(n_samples, 5) | ||
|
|
||
| np.save(device_root / "timestamps.npy", timestamps) | ||
| np.save(device_root / "data.npy", data) | ||
|
|
||
| meta = { | ||
| "start_time": start, | ||
| "end_time": end, | ||
| "modality": "sequence", | ||
| "sampling_rate": sampling_rate, | ||
| "phase_shift_per_signal": False, | ||
| "is_mem_mapped": False, | ||
| "n_signals": 5, | ||
| "n_timestamps": n_samples, | ||
| "dtype": "float64", | ||
| } | ||
| with open(device_root / "meta.yml", "w") as f: | ||
| yaml.safe_dump(meta, f) | ||
|
|
||
| yield EXPERIMENT_ROOT | ||
|
|
||
| finally: | ||
| shutil.rmtree(EXPERIMENT_ROOT) | ||
|
|
|
|
||
| jobs: | ||
| ruff: | ||
| if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true) |
Comment on lines
1
to
9
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| import re | ||
| import warnings | ||
| from collections.abc import Sequence | ||
| from pathlib import Path | ||
| from typing import Optional, Union | ||
| from typing import Union | ||
|
|
||
| import numpy as np | ||
| from hydra.utils import instantiate |
Contributor
|
please, re-submit a PR making changed from the current main only, not from your other branch |
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.
Closes #134
Description
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
Updated
_load_devices()to usemin/maxto track the earliest start and latest end across all devices.Test
Added
tests/test_experiment.py3 tests:All 363 tests passing.