Add test coverage for Experiment class and data generation utilities

[BitForge95]

Fixes points a and b of #76

Hi @pollytur and @reneburghardt,

Following up on my progress in the issue thread, I’m opening this PR to add the test coverage for experiment.py. I've focused on making sure the Experiment class can handle the real-world data issues we discussed, like non-zero start times and irregular sampling.

What I’ve done:

• Experiment Class: I updated the class to support three ways of getting an interpolator: Hydra, existing objects, or the original logic. I also added logic to track the global start and end times across all devices.
• Data Generator: I built create_sequence_data.py to create temporary test folders. This was key for testing specific problems without needing real files. I added a start_time parameter for testing offsets (like starting at 1.5s) and an irregular flag to simulate sensor jitter.
• Environment Setup: I used a context manager in create_experiment.py to handle the setup and teardown of the test folders. It makes sure everything is cleaned up after the tests run so the project directory stays tidy.

Testing Strategy:

I split the tests in test_experiment.py into two parts:

1. Routing logic: I used mocks to make sure the Experiment class is sending data to the right interpolators.
2. Integration: I used the generator to verify that the code handles real file inputs and calculates valid ranges correctly.

To handle the numeric problems mentioned in the thread, I used pytest.approx for time comparisons and tested the code with non-integer sampling rates (like 33.33 Hz) to ensure the math stays stable.

Thanks again to @reneburghardt for the help with the initial structure!

[gitnotebooks]

Review these changes at https://app.gitnotebooks.com/sensorium-competition/experanto/pull/113

[Copilot]

Pull request overview

This PR adds new tests and test-data utilities to improve coverage around experanto.experiment.Experiment, specifically targeting real-world timing issues (non-zero starts, numeric precision, and irregular sampling), and updates Experiment to support multiple interpolator construction paths.

Changes:

• Add tests/test_experiment.py with routing-focused unit tests plus disk-backed integration tests for valid ranges and time offsets.
• Refactor sequence test-data generation (tests/create_sequence_data.py) and add tests/create_experiment.py for creating multi-device experiment folders in tests.
• Update experanto/experiment.py device loading to support Hydra-instantiated interpolators, pre-instantiated interpolator objects, and a fallback construction path; also track global start/end across devices.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
tests/test_experiment.py	New unit + integration tests for Experiment routing and time-range behaviors.
tests/create_sequence_data.py	Refactors sequence-data generation and cleanup helpers for tests.
tests/create_experiment.py	New helper/context manager to generate temporary multi-device experiment folder structures.
experanto/experiment.py	Updates interpolator instantiation logic and global start/end tracking; minor API/behavior tweaks.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[BitForge95]

@copilot open a new pull request to apply changes based on the comments in this thread

[reneburghardt]

@copilot open a new pull request to apply changes based on the comments in this thread

I am not fully sure if it is still the case, but some weeks ago this was not possible for PRs from a fork.

[BitForge95]

Ah, okay! I'll do that manually then. Thanks for the fork limitations.

[BitForge95]

Hi @pollytur and @reneburghardt, I have manually applied all the suggestions. Tests are passing locally and the PR is now up to date.

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
experanto/experiment.py	75.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[pollytur]

please apply pyright, black, isort - otherwise things dont pass CI / CD

• see a comment above

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[BitForge95]

Hi @pollytur and @reneburghardt, just checking in on this!

I left some replies under the Copilot suggestions a couple of days ago, but to save you from digging through the inline threads, here is a quick summary of the logic I'd like to update:

1. Test Concurrency: Refactor create_experiment to use pytest's tmp_path fixture instead of a hardcoded path.
2. Irregular Timestamps: Delete the test_experiment_irregular_timestamps test and omit the irregular parameter from the API, since SequenceInterpolator doesn't natively support jitter yet anyway.
3. Resource Leaks: Wrap the interpolator creation in a with block to prevent Windows file locks (I verified the base Interpolator safely supports __enter__ and __exit__).
4. API Threading: Add start_time to the public create_sequence_data() wrapper so tests can actually use it.

If this logic looks good to you, just give me a thumbs up and I'll push the updates!

[pollytur]

@BitForge95 thanks for the reminder - I will check through the comments tonight or tomorrow (sorry its been two big PRs last week)
In the meanwhile please 1) merge current main into your branch (resolving merge conflicts) and 2) fix the pyright (currently breaking check in CI/CD)

[BitForge95]

Hey @pollytur and @reneburghardt, thanks for the clear directions. I just force-pushed the latest updates. I merged main and resolved all the conflicts, and Pyright is now completely passing locally for the files I modified. I also implemented all the structural changes we discussed: switching to tmp_path, wrapping the interpolators in a try...finally block, adding the length assertion, exposing the start_time parameter, and removing the redundant irregular timestamps test. Let me know if you need any other tweaks.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

experanto/experiment.py:119

• warnings.warn("Falling back to original Interpolator creation logic") currently runs for the normal case where modality_config[device]["interpolation"] is a plain dict (e.g., configs/default.yaml), which will make typical usage noisy and can fail if warnings are treated as errors. After deduplicating the create call, treat dict/DictConfig without _target_ as the standard path (no warning), and reserve warnings/errors for unsupported types.

                # Default back to original logic
                warnings.warn(
                    "Falling back to original Interpolator creation logic.",
                    UserWarning,
                )

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[BitForge95]

Hey @pollytur and @reneburghardt, I just pushed the final updates. I removed the unused irregular flag that we discussed and cleaned up the redundant imports and docstrings caught by the Copilot reviews. All tests and Pyright checks are completely passing locally. Let me know if there is anything else you need before merging.

[pollytur]

@BitForge95 could you please merge main into your current branch and also please resolve the conversations above is there are solved in agreement
then rerequest the review, such that we get a email that its ready to look at

[BitForge95]

I just merged the latest changes from main, resolved all the conversation threads, and re-requested a review. Everything should be ready for you to look at.

[pollytur]

@BitForge95 I dont think you did - there is still a merge conflict
also note that logging in the experiment.py was changed a bit

[BitForge95]

My mistake! My local main was behind. I just synced upstream, pulled, and properly merged main, making sure to keep your updated logger format and time assignment logic in experiment.py. The conflicts are completely cleared out now.

[reneburghardt]

@BitForge95 I can see recent changes from e.g. #147 are reversed here (for example removing workflow_dispatch from all CI pipelines). Is that on purpose and if so, what is the reason?

[BitForge95]

Hi @reneburghardt, this was definitely not on purpose. I recently did a rebase and adjusted my commits to clean up the PR history, and my local working directory was out of sync. As a result, I accidentally reverted some of the recent updates from main (like #147 and the interpolator changes). I am fixing this right now to restore all those files so this PR contains only the test coverage additions. Apologies for the confusion.

[BitForge95]

@reneburghardt I have restored the CI workflows , pyproject.toml, and interpolators.py.

[BitForge95]

@reneburghardt i have revereted the changes.

[pollytur]

@BitForge95 why dont we see the CI/CD checks anymore? also if I check it on your branch - ruff checks actually do not pass
@reneburghardt do you have an idea about CI / CD?

[reneburghardt]

@BitForge95 why dont we see the CI/CD checks anymore? also if I check it on your branch - ruff checks actually do not pass @reneburghardt do you have an idea about CI / CD?

Its always a bit weird when the last commit is from the auto-format bot, but beyond that I have no idea currently

[BitForge95]

Hi @pollytur and @reneburghardt. The CI checks disappeared because the last commit on this branch was made automatically by the black/isort formatting bot. GitHub's default security settings prevent commits made by a bot's GITHUB_TOKEN from triggering subsequent workflows (to prevent infinite Action loops).If you want I can pull the bot's changes, fix the ruff errors locally, and push a new commit. Since this push came from my user account, the CI pipelines should all be re-triggered and visible now.

[BitForge95]

@pollytur @reneburghardt Ruff was flagging B905 because I removed strict=True from the zip() functions. I had to remove strict=True to keep the tests compatible with Python 3.9 (which threw a TypeError in pytest). I added a # noqa: B905 to bypass the linter for those two lines so the CI can pass, but please let me know if you'd prefer i handle that 3.9 compatibility differently,

[reneburghardt]

@BitForge95 Can you run tests with 3.12.8 (as in

experanto/.github/workflows/test.yml

Line 31 in d851484

python-version: '3.12.8'

)? This should then allow to solve ruff issues, right?
Lets not worry about backcompatibility to 3.9 here (IMO); we might get rid of 3.9 anyway soon and if not, it needs to be handled in a different PR I think

Another thing that also came to my mind: we basically now have to two functions in create_experiment.py for returning config (get_default_config and make_modality_config) and make_sequence_device is also similar to _generate_sequence_data from create_sequence_data.py. Can we get rid of one config function and maybe make_modality_config? Otherwise we have similar functions that we all have to maintain in the future. I know this redundancy happened due to another PR, but would be nice if we can solve it here.

[BitForge95]

Thanks @reneburghardt. I'll bump the CI to 3.12.8. I'll refactor the tests right now to use setup_test_experiment (with _generate_sequence_data) and get_default_config, and remove the redundant functions. I'll push those updates shortly.

[reneburghardt]

Thanks @reneburghardt. I'll bump the CI to 3.12.8. I'll refactor the tests right now to use setup_test_experiment (with _generate_sequence_data) and get_default_config, and remove the redundant functions. I'll push those updates shortly.

The CI is already at 3.12.8, but you mentioned 3.9 for tests, hence I assumed you ran tests locally with 3.9 leading to the issues you described

[BitForge95]

Sorry , I meant locally . my bad

[BitForge95]

@reneburghardt I just pushed the refactored tests using setup_test_experiment and get_default_config, and completely removed the redundant helper functions. Let me know if i need to do any more changes.

[binary69]

Hey @reneburghardt just for the clarification, for test_experiment_start_end_time_reflects_union which is parametrized over 1, 2, and 3+ devices, get_default_config is hardcoded for exactly 2 devices named device_0 and device_1. How does the refactored version handle the 1-device and 3-device cases without make_modality_config

[binary69]

I noticed pytest.approx is used for the start_time/end_time comparisons here, is that intentional? I ask because in #137 the feedback was to use exact equality since the values come directly from meta.yml with no numerics involved. Happy to learn if there's a reason to prefer approx here:)

[BitForge95]

Hi @binary69 , In the refactored test_experiment_start_end_time_reflects_union, I actually stopped using get_default_config for that exact reason. Instead, the test builds the config dynamically based on however many devices are passed in, so it scales perfectly to 1, 3, or N devices.

        for i in range(len(device_ranges)):
            config[f"device_{i}"] = {
                "interpolation": {
                    "sampling_rate": 10.0,
                    "offset": float(np.random.rand()),
                }
            }

        experiment = Experiment(
            root_folder=str(experiment_path), modality_config=config
        )

By looking at the #137 , you are right . I originally used it out of habit to stay safe against floating-point drift. Exact equality is preferred in this case. Thanks for linking #137 for context

[reneburghardt]

@BitForge95 Can you run tests with 3.12.8 (as in

experanto/.github/workflows/test.yml

Line 31 in d851484

python-version: '3.12.8'

)? This should then allow to solve ruff issues, right?
Lets not worry about backcompatibility to 3.9 here (IMO); we might get rid of 3.9 anyway soon and if not, it needs to be handled in a different PR I think
Another thing that also came to my mind: we basically now have to two functions in create_experiment.py for returning config (get_default_config and make_modality_config) and make_sequence_device is also similar to _generate_sequence_data from create_sequence_data.py. Can we get rid of one config function and maybe make_modality_config? Otherwise we have similar functions that we all have to maintain in the future. I know this redundancy happened due to another PR, but would be nice if we can solve it here.

Sorry I just saw that I confused make_modality_config with make_sequence_device here. Originally, I wanted to keep either one of the config functions (so either get_default_config or make_modality_config, whatever makes more sense) and remove make_sequence_device if possible and use _generate_sequence_data.

So if I see it correctly after a quick glance, due to my not well-written comment we now have get_default_config and another mechanism without a function to loop over multiple devices, right? As this are then still two different places of similar functionality which need to be maintained, I'd say we use make_modality_config instead of get_default_config (and extend it if necessary). What do you think @BitForge95 ? Sorry for not checking my earlier comment properly

[BitForge95]

@reneburghardt , no worries. Now this makes this clear. make_modality_config is definitely the cleaner choice since it dynamically scales to any number of devices. I've just pushed a commit that removes get_default_config and refactors the tests to rely purely on make_modality_config and setup_test_experiment. Also according to the #137 I also removed pytest.approx in favor of exact equality == across the board in this latest push. Let me know how this looks ?