-
Notifications
You must be signed in to change notification settings - Fork 36
fix: compute Experiment start/end time as union across all devices #137
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
Merged
schewskone
merged 26 commits into
sensorium-competition:main
from
binary69:fix/experiment-global-time-range
Mar 20, 2026
Merged
Changes from 22 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
ec61bab
fix: compute Experiment start/end time as union across all devices
binary69 1008758
test: use pytest tmp_path for cleaner test isolation
binary69 118a928
style: auto-format with black and isort
invalid-email-address 36e6707
fix: add None guard for dev.start_time and end_time in _load_devices
binary69 45c6881
test: separate data creation helpers into create_experiment.py
binary69 38153ad
style: auto-format with black and isort
invalid-email-address 791bb98
fix: improve None handling with warning and add finite range validation
binary69 d6737de
style: auto-format with black and isort
invalid-email-address 7c07754
fix for pull request finding
binary69 dbf1896
fix for pull request finding
binary69 55e8f2e
style: auto-format with black and isort
invalid-email-address a068290
style: reformat elif condition to Black-compliant style
binary69 ae606bd
style: auto-format with black and isort
invalid-email-address 929c92d
fix: experiment time range validation and test refactor
binary69 a7edf85
fix: remove unused imports flagged by ruff
binary69 423f86f
style: auto-format with black and isort
invalid-email-address 86dc0bc
fix: replace np.isinf with np.isfinite to catch NaN and -inf values
binary69 3f4aa6e
fix: make sampling_rates and offsets per-device lists in make_modalit…
binary69 2e3cddf
fix: expand test coverage with edge cases, parametrize invalid device…
binary69 3c2ccc3
style: auto-format with black and isort
invalid-email-address 863e52c
fix: restore imports to match main branch
binary69 acbfd6c
style: auto-format with black and isort
invalid-email-address 17f752e
merge: sync with upstream main and resolve experiment.py conflict
binary69 33828ea
merge: resolve experiment.py conflict keeping upstream main imports
binary69 63f7589
fix: use correct sampling_rate parameter and random values in union test
binary69 2488615
style: auto-format with black and isort
invalid-email-address File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import shutil | ||
| from contextlib import contextmanager | ||
|
|
||
| import numpy as np | ||
| import yaml | ||
|
|
||
|
|
||
|
binary69 marked this conversation as resolved.
|
||
| @contextmanager | ||
| def make_sequence_device( | ||
| root, name, start, end, sampling_rate=10.0, n_signals=5, override_meta=None | ||
| ): | ||
| """Create a single sequence device folder under root.""" | ||
| device_root = root / name | ||
| try: | ||
| (device_root / "meta").mkdir(parents=True, exist_ok=True) | ||
|
|
||
| n_samples = ( | ||
| int((end - start) * sampling_rate) + 1 | ||
| ) # +1 to include both start and end as sample points | ||
| timestamps = np.linspace(start, end, n_samples) | ||
| data = np.random.rand(n_samples, n_signals) | ||
|
|
||
| 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": n_signals, | ||
| "n_timestamps": n_samples, | ||
| "dtype": "float64", | ||
| } | ||
| if override_meta: | ||
| meta.update(override_meta) | ||
| with open(device_root / "meta.yml", "w") as f: | ||
| yaml.safe_dump(meta, f) | ||
|
|
||
| yield device_root | ||
|
|
||
| finally: | ||
| shutil.rmtree(device_root) | ||
|
|
||
|
|
||
| def make_modality_config(*device_names, sampling_rates=None, offsets=None): | ||
| if sampling_rates is None: | ||
| sampling_rates = [10.0] * len(device_names) | ||
| if offsets is None: | ||
| offsets = [0.0] * len(device_names) | ||
|
binary69 marked this conversation as resolved.
|
||
|
|
||
| assert len(device_names) == len( | ||
| sampling_rates | ||
| ), f"sampling_rates length {len(sampling_rates)} does not match device_names length {len(device_names)}" | ||
| assert len(device_names) == len( | ||
| offsets | ||
| ), f"offsets length {len(offsets)} does not match device_names length {len(device_names)}" | ||
|
|
||
| return { | ||
| name: {"interpolation": {"sampling_rate": sr, "offset": off}} | ||
| for name, sr, off in zip(device_names, sampling_rates, offsets) | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| import logging | ||
| from contextlib import ExitStack | ||
|
|
||
| import pytest | ||
|
|
||
| from experanto.experiment import Experiment | ||
|
|
||
| from .create_experiment import make_modality_config, make_sequence_device | ||
|
|
||
| DEVICE_TIME_RANGE_CASES = [ | ||
| # Single device: start and end should match that device's range | ||
| ([(2.0, 9.0)], 2.0, 9.0), | ||
| # Two devices with different ranges: start should be min, end should be max | ||
| ([(1.0, 8.0), (0.0, 10.0)], 0.0, 10.0), | ||
| # Three devices with different ranges: start should be min, end should be max | ||
| ([(0.0, 10.0), (1.0, 8.0), (2.0, 9.0)], 0.0, 10.0), | ||
| # Devices with non-overlapping ranges: start should be min, end should be max | ||
| ([(0.0, 3.0), (7.0, 8.0)], 0.0, 8.0), | ||
| # Devices with identical ranges: start and end should match that range | ||
| ([(1.0, 5.0), (1.0, 5.0)], 1.0, 5.0), | ||
| # Large time stamps: start should be min, end should be max | ||
| ([(1e9, 1e9 + 100), (1e9 - 50, 1e9 + 50)], 1e9 - 50, 1e9 + 100), | ||
| ] | ||
|
|
||
| DEVICE_TIME_RANGE_IDS = [ | ||
| "single_device", | ||
| "two_devices_different_ranges", | ||
| "three_devices_different_ranges", | ||
| "non_overlapping_ranges", | ||
| "identical_ranges", | ||
| "large_time_stamps", | ||
| ] | ||
|
|
||
| # Inverted range is intentionally separate from INVALID_META_CASES — | ||
| # None/NaN/inf are caught per-device before being added to self.devices, | ||
| # whereas start > end is only caught after all devices are loaded. | ||
| INVALID_META_CASES = [ | ||
| {"start_time": None, "end_time": None}, # Both missing | ||
| {"start_time": None, "end_time": 10.0}, # Missing start_time | ||
| {"start_time": 0.0, "end_time": None}, # Missing end_time | ||
| {"start_time": float("inf"), "end_time": 10.0}, # Infinite start_time | ||
| {"start_time": 0.0, "end_time": float("inf")}, # Infinite end_time | ||
| {"start_time": float("-inf"), "end_time": 10.0}, # Negative Infinite start_time | ||
| {"start_time": 0.0, "end_time": float("-inf")}, # Negative Infinite end_time | ||
| {"start_time": float("nan"), "end_time": 10.0}, # NaN start_time | ||
| {"start_time": 0.0, "end_time": float("nan")}, # NaN end_time | ||
| ] | ||
|
|
||
| INVALID_META_IDS = [ | ||
| "both_missing", | ||
| "missing_start_time", | ||
| "missing_end_time", | ||
| "infinite_start_time", | ||
| "infinite_end_time", | ||
| "negative_infinite_start_time", | ||
| "negative_infinite_end_time", | ||
| "nan_start_time", | ||
| "nan_end_time", | ||
| ] | ||
|
|
||
|
|
||
| # Test for union of device time ranges | ||
| @pytest.mark.parametrize("n_signals", [5, 20]) | ||
| @pytest.mark.parametrize( | ||
| "device_ranges, expected_start, expected_end", | ||
| DEVICE_TIME_RANGE_CASES, | ||
| ids=DEVICE_TIME_RANGE_IDS, | ||
| ) | ||
| def test_experiment_start_end_time_reflects_union( | ||
| tmp_path, device_ranges, expected_start, expected_end, n_signals | ||
| ): | ||
| """ | ||
| Experiment.start_time and end_time should reflect the union of all | ||
| device time ranges — earliest start and latest end across all devices. | ||
| """ | ||
| device_names = [f"device_{i}" for i in range(len(device_ranges))] | ||
|
|
||
| with ExitStack() as stack: | ||
| for name, (start, end) in zip(device_names, device_ranges): | ||
| stack.enter_context( | ||
| make_sequence_device( | ||
| tmp_path, name, start=start, end=end, n_signals=n_signals | ||
|
binary69 marked this conversation as resolved.
Outdated
|
||
| ) | ||
| ) | ||
|
|
||
| experiment = Experiment( | ||
| root_folder=tmp_path, | ||
| modality_config=make_modality_config(*device_names), | ||
| ) | ||
|
|
||
| assert experiment.start_time == pytest.approx( | ||
| expected_start | ||
| ), f"Expected start_time={expected_start}, got {experiment.start_time}" | ||
| assert experiment.end_time == pytest.approx( | ||
| expected_end | ||
| ), f"Expected end_time={expected_end}, got {experiment.end_time}" | ||
|
binary69 marked this conversation as resolved.
Outdated
|
||
|
|
||
|
binary69 marked this conversation as resolved.
|
||
|
|
||
| # Safety check | ||
| @pytest.mark.parametrize("override_meta", INVALID_META_CASES, ids=INVALID_META_IDS) | ||
| def test_experiment_invalid_metadata(tmp_path, override_meta): | ||
| """ | ||
| Experiment should raise an error when initialized with invalid metadata. | ||
| Covers cases where start_time or end_time is None, NaN, or infinite. | ||
| """ | ||
| with make_sequence_device( | ||
| tmp_path, | ||
| "device_0", | ||
| start=0.0, | ||
| end=10.0, | ||
| override_meta=override_meta, | ||
| ): | ||
| with pytest.raises( | ||
| ValueError, match="Experiment time range could not be determined" | ||
| ): | ||
| Experiment( | ||
| root_folder=tmp_path, | ||
| modality_config=make_modality_config("device_0"), | ||
| ) | ||
|
|
||
|
|
||
| def test_experiment_inverted_time_range_raises(tmp_path): | ||
| """ | ||
| Experiment should raise ValueError when start_time > end_time. | ||
| This is a separate guard from invalid metadata (None/NaN/inf) because it | ||
| only becomes apparent after all devices are loaded and the overall time range is computed. | ||
| """ | ||
| with make_sequence_device( | ||
| tmp_path, | ||
| "device_0", | ||
| start=0.0, | ||
| end=10.0, | ||
| override_meta={"start_time": 5.0, "end_time": 2.0}, | ||
| ): | ||
| with pytest.raises( | ||
| ValueError, match="Experiment time range could not be determined" | ||
| ): | ||
| Experiment( | ||
| root_folder=tmp_path, | ||
| modality_config=make_modality_config("device_0"), | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("override_meta", INVALID_META_CASES, ids=INVALID_META_IDS) | ||
| def test_experiment_skips_invalid_devices(tmp_path, override_meta, caplog): | ||
| """ | ||
| Experiment should skip devices with invalid start_time or end_time and | ||
| log a warning, but still initialize successfully if at least one valid | ||
| device is present. The experiment time range should reflect only the | ||
| valid device. | ||
| """ | ||
| with ExitStack() as stack: | ||
| # Valid device with proper metadata | ||
| stack.enter_context( | ||
| make_sequence_device( | ||
| tmp_path, | ||
| "valid_device", | ||
| start=0.0, | ||
| end=10.0, | ||
| ) | ||
| ) | ||
| # Invalid device with missing start_time and end_time | ||
| stack.enter_context( | ||
| make_sequence_device( | ||
| tmp_path, | ||
| "invalid_device", | ||
| start=0.0, | ||
| end=10.0, | ||
| override_meta=override_meta, | ||
| ) | ||
| ) | ||
|
|
||
| with caplog.at_level(logging.WARNING, logger="experanto.experiment"): | ||
| experiment = Experiment( | ||
| root_folder=tmp_path, | ||
| modality_config=make_modality_config("valid_device", "invalid_device"), | ||
| ) | ||
|
|
||
| assert "valid_device" in experiment.devices | ||
| assert "invalid_device" not in experiment.devices | ||
|
|
||
| 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}" | ||
| assert any( | ||
| "invalid_device" in message for message in caplog.messages | ||
| ), "Expected warning about invalid_device was skipped" | ||
|
binary69 marked this conversation as resolved.
|
||
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.
Uh oh!
There was an error while loading. Please reload this page.