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 75.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.

[BitForge95]

@pollytur, the CI just failed on the Run Tests step, specifically on test_linear_interpolation.

Looking at the logs, it seems to be a statistical edge case: because contain_nans=True, the generator occasionally produces a column that is 100% NaNs. Since np.nanmean can't impute an entirely empty column, the final isnan().sum() == 0 assertion fails.

Would you like me to push a quick update to the test to exclude fully NaN columns from that final check so the CI passes, or would you prefer to handle that edge case differently?

[pollytur]

@BitForge95 could you please add some logs screenshot for the nans behaviour that you describe above?
and the pseudocode for the suggested fix

[BitForge95]

Here is the screenshot of the CI logs showing the failure.

Because contain_nans=True, the generator happened to create a sequence where the second signal (column) was entirely NaNs. You can see in the boolean array output at the bottom that the second column is True all the way down.

Since np.nanmean cannot impute a column that is 100% NaN, those NaNs correctly remain in the interp array, which causes the final blanket sum() == 0 assertion to fail.

The Suggested Fix:
We just need to calculate which columns were 100% NaN in the expected data, and exclude those specific columns from the final assertion.

    if not keep_nans:
            # --- Current ---
            # assert np.isnan(interp).sum() == 0
            
            # --- Suggested Fix ---
            # Identify columns that are entirely NaN and cannot be imputed
            all_nan_cols = np.all(np.isnan(expected), axis=0)
            
            # Assert 0 NaNs only in the columns that were capable of being imputed
            assert (
                np.isnan(interp[:, ~all_nan_cols]).sum() == 0
            ), "Imputable signals should not contain NaNs"

[pollytur]

@BitForge95 seems like the return in create_sequence_data.py‎ was needed cause now a lot of tests fail https://github.com/BitForge95/experanto/actions/runs/24388879059/job/71229619076

[pollytur]

@BitForge95 thanks a lot once again!