Address issues

[pollytur]

Small PR to address issues #32 , #70 , #73

[gitnotebooks]

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

[Copilot]

Pull request overview

This PR addresses issues #32, #70, and #73 by introducing a return_valid parameter to interpolation methods, removing unused code, and renaming a confusing transform option. The changes enable callers to optionally request validation masks from interpolation operations, making the API more flexible while maintaining backward compatibility through default parameter values.

Key Changes:

• Added return_valid parameter to all interpolator classes to conditionally return validation masks
• Renamed "standardize" transform mode to "normalize_variance_only" for clarity
• Removed unused namedtuple imports and DataPoint class

Reviewed changes

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

Show a summary per file

File	Description
experanto/interpolators.py	Added return_valid parameter to abstract method and all concrete implementations (SequenceInterpolator, PhaseShiftedSequenceInterpolator, ScreenInterpolator, TimeIntervalInterpolator) with conditional return logic
experanto/experiment.py	Updated interpolate method signature to accept and propagate return_valid parameter; removed unused namedtuple import
experanto/datasets.py	Updated interpolate calls to use return_valid=False; renamed "standardize" to "normalize_variance_only"; removed unused namedtuple and DataPoint
configs/default.yaml	Updated normalization config value from "standardize" to "normalize_variance_only"
docs/source/concepts/demo_configs.rst	Updated example config to use "normalize_variance_only"
README.md	Added documentation badge link

---

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

[Copilot]

When return_valid is False, interp.interpolate() will return only the values (not a tuple), causing an unpacking error. The code attempts to unpack into two variables (values[d], valid[d]) but will receive only one value when return_valid=False. This should be handled conditionally similar to how it's done in lines 73-75 and 76-79.

Suggested change

	values[d], valid[d] = interp.interpolate(
	times, return_valid=return_valid
	)
	if return_valid:
	values[d], valid[d] = interp.interpolate(
	times, return_valid=return_valid
	)
	else:
	values[d] = interp.interpolate(
	times, return_valid=return_valid
	)

[Copilot]

When return_valid is False, the interpolate call will return only the values (not a tuple), causing an unpacking error. The code attempts to unpack into two variables (values, valid) but will receive only one value when return_valid=False. Consider handling this conditionally to match the actual return behavior.

[Copilot]

Pull request overview

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

---

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

[Copilot]

The type annotation for the device parameter is Interpolator | None, but the code at line 79 checks isinstance(device, str), indicating that device can also be a string (device name). The type annotation should be str | Interpolator | None or Union[str, Interpolator, None] to accurately reflect the accepted types.

Suggested change

	device: Interpolator | None = None,
	device: Union[str, Interpolator, None] = None,

[Copilot]

The return type annotation Union[tuple[np.ndarray, np.ndarray], np.ndarray] is incorrect. When device is None, the function returns dictionaries (either just values dict or (values, valid) tuple of dicts), not numpy arrays. The return type should be Union[tuple[dict, dict], dict, tuple[np.ndarray, np.ndarray], np.ndarray] or a more specific type that accurately represents both the dict and array cases.

[Copilot]

The return statement at line 589 correctly handles the return_valid parameter, but line 564 (the early return when n_times == 0) does not respect this parameter. This creates inconsistent return types depending on whether the times array is empty or not. While line 564 is outside the diff region, the inconsistency is introduced by adding the return_valid parameter without updating all return paths.

[Copilot]

When device is None, the function builds values and potentially valid dictionaries, but never returns them. The function should return (values, valid) when return_valid=True or just values when return_valid=False at the end of this branch.

Suggested change

	values[d] = res
	values[d] = res
	if return_valid:
	return values, valid
	else:
	return values

[Copilot]

The docstring does not document the new return_valid parameter that was added to the function signature. The parameter should be documented in the Parameters section, and the Returns section should be updated to reflect that the return type varies based on this parameter.

[Copilot]

The new return_valid parameter is not tested in any of the TimeIntervalInterpolator tests. Tests should be added to verify that when return_valid=True, the function returns a tuple of (output, valid_mask), and when return_valid=False (the default), it returns only the output array.

[Copilot]

The abstract method signature now includes the return_valid parameter, but there's no docstring explaining what this parameter does. Since this is an abstract method that all interpolators must implement, it should have comprehensive documentation explaining the parameter and return value behavior.

Suggested change

	...
	# returns interpolated signal and boolean mask of valid samples
	"""
	Interpolate the signal at the specified time points.
	Parameters
	----------
	times : np.ndarray
	Array of time points at which to interpolate the signal.
	return_valid : bool, optional
	If True, also return a boolean mask indicating which samples are valid.
	If False (default), only the interpolated signal is returned.
	Returns
	-------
	np.ndarray or tuple of (np.ndarray, np.ndarray)
	If return_valid is False, returns the interpolated signal as a numpy array.
	If return_valid is True, returns a tuple containing the interpolated signal and
	a boolean numpy array mask of valid samples.
	"""
	...