From 10c2b53396cf532b1efb51382cb8933a3d610fac Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Wed, 23 Oct 2024 09:52:04 -0400 Subject: [PATCH 01/14] require sample axis to be axis=0 --- src/nemos/basis.py | 53 +++++++++++++++++++-------- tests/test_basis.py | 89 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 15 deletions(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index 1b0c9f12..ee6d5a20 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -5,6 +5,7 @@ import abc import copy +import inspect from functools import wraps from typing import Callable, Generator, Literal, Optional, Tuple, Union @@ -468,10 +469,19 @@ class Basis(Base, abc.ABC): bounds : The bounds for the basis domain in `mode="eval"`. The default `bounds[0]` and `bounds[1]` are the minimum and the maximum of the samples provided when evaluating the basis. - If a sample is outside the bonuds, the basis will return NaN. + If a sample is outside the bounds, the basis will return NaN. **kwargs : Only used in "conv" mode. Additional keyword arguments that are passed to `nemos.convolve.create_convolutional_predictor` + + Raises + ------ + ValueError: + - If `mode` is not 'eval' or 'conv'. + - If `kwargs` are not None and `mode =="eval". + - If `kwargs` include parameters not recognized or do not have + default values in `create_convolutional_predictor`. + - If `axis` different from 0 is provided as a keyword argument (samples must always be in the first axis). """ def __init__( @@ -501,6 +511,26 @@ def __init__( f"kwargs should only be set when mode=='conv', but '{mode}' provided instead!" ) + # check on convolution kwargs content + if "axis" in kwargs and kwargs["axis"] != 0: + raise ValueError( + f"Invalid `axis={kwargs['axis']}` provided. This basis requires the " + f"convolution to be applied along the first axis (`axis=0`).\n" + "Please transpose your input so that the desired axis for " + "convolution is the first dimension (axis=0)." + ) + convolve_params = inspect.signature(create_convolutional_predictor).parameters + convolve_configs = { + key for key, param in convolve_params.items() + if param.default is not inspect.Parameter.empty + } + if not set(kwargs.keys()).issubset(convolve_configs): + invalid = set(kwargs.keys()).difference(convolve_configs) + raise ValueError( + f"Unrecognized keyword arguments: {invalid}. " + f"Allowed convolution keyword arguments are: {convolve_configs}." + ) + self.kernel_ = None self._identifiability_constraints = False @@ -666,18 +696,11 @@ def _compute_features(self, *xi: ArrayLike) -> FeatureMatrix: if self.mode == "eval": # evaluate at the sample return self.__call__(*xi) else: # convolve, called only at the last layer - if "axis" not in self._conv_kwargs: - axis = 0 - else: - axis = self._conv_kwargs["axis"] # convolve called at the end of any recursive call # this ensures that len(xi) == 1. conv = create_convolutional_predictor( self.kernel_, *xi, **self._conv_kwargs ) - # move the time axis to the first dimension - new_axis = (np.arange(conv.ndim) + axis) % conv.ndim - conv = np.transpose(conv, new_axis) # make sure to return a matrix return np.reshape(conv, newshape=(conv.shape[0], -1)) @@ -1289,7 +1312,7 @@ class SplineBasis(Basis, abc.ABC): bounds : The bounds for the basis domain in `mode="eval"`. The default `bounds[0]` and `bounds[1]` are the minimum and the maximum of the samples provided when evaluating the basis. - If a sample is outside the bonuds, the basis will return NaN. + If a sample is outside the bounds, the basis will return NaN. **kwargs : Only used in "conv" mode. Additional keyword arguments that are passed to `nemos.convolve.create_convolutional_predictor` @@ -1440,7 +1463,7 @@ class MSplineBasis(SplineBasis): bounds : The bounds for the basis domain in `mode="eval"`. The default `bounds[0]` and `bounds[1]` are the minimum and the maximum of the samples provided when evaluating the basis. - If a sample is outside the bonuds, the basis will return NaN. + If a sample is outside the bounds, the basis will return NaN. **kwargs: Only used in "conv" mode. Additional keyword arguments that are passed to `nemos.convolve.create_convolutional_predictor` @@ -1598,7 +1621,7 @@ class BSplineBasis(SplineBasis): bounds : The bounds for the basis domain in `mode="eval"`. The default `bounds[0]` and `bounds[1]` are the minimum and the maximum of the samples provided when evaluating the basis. - If a sample is outside the bonuds, the basis will return NaN. + If a sample is outside the bounds, the basis will return NaN. **kwargs : Only used in "conv" mode. Additional keyword arguments that are passed to `nemos.convolve.create_convolutional_predictor` @@ -1717,7 +1740,7 @@ class CyclicBSplineBasis(SplineBasis): bounds : The bounds for the basis domain in `mode="eval"`. The default `bounds[0]` and `bounds[1]` are the minimum and the maximum of the samples provided when evaluating the basis. - If a sample is outside the bonuds, the basis will return NaN. + If a sample is outside the bounds, the basis will return NaN. **kwargs : Only used in "conv" mode. Additional keyword arguments that are passed to `nemos.convolve.create_convolutional_predictor` @@ -1859,7 +1882,7 @@ class RaisedCosineBasisLinear(Basis): bounds : The bounds for the basis domain in `mode="eval"`. The default `bounds[0]` and `bounds[1]` are the minimum and the maximum of the samples provided when evaluating the basis. - If a sample is outside the bonuds, the basis will return NaN. + If a sample is outside the bounds, the basis will return NaN. **kwargs : Only used in "conv" mode. Additional keyword arguments that are passed to `nemos.convolve.create_convolutional_predictor` @@ -2052,7 +2075,7 @@ class RaisedCosineBasisLog(RaisedCosineBasisLinear): bounds : The bounds for the basis domain in `mode="eval"`. The default `bounds[0]` and `bounds[1]` are the minimum and the maximum of the samples provided when evaluating the basis. - If a sample is outside the bonuds, the basis will return NaN. + If a sample is outside the bounds, the basis will return NaN. **kwargs : Only used in "conv" mode. Additional keyword arguments that are passed to `nemos.convolve.create_convolutional_predictor` @@ -2206,7 +2229,7 @@ class OrthExponentialBasis(Basis): bounds : The bounds for the basis domain in `mode="eval"`. The default `bounds[0]` and `bounds[1]` are the minimum and the maximum of the samples provided when evaluating the basis. - If a sample is outside the bonuds, the basis will return NaN. + If a sample is outside the bounds, the basis will return NaN. **kwargs : Only used in "conv" mode. Additional keyword arguments that are passed to `nemos.convolve.create_convolutional_predictor` diff --git a/tests/test_basis.py b/tests/test_basis.py index 3e21db33..dfb23c53 100644 --- a/tests/test_basis.py +++ b/tests/test_basis.py @@ -495,6 +495,23 @@ def test_init_mode(self, mode, expectation): with expectation: self.cls(5, mode=mode, window_size=window_size) + @pytest.mark.parametrize( + "conv_kwargs, expectation", + [ + (dict(), does_not_raise()), + (dict(axis=0), does_not_raise()), + (dict(axis=1), pytest.raises(ValueError, match="Invalid `axis=1` provided")), + (dict(shift=True), does_not_raise()), + (dict(shift=True, axis=0), does_not_raise()), + (dict(shifts=True, axis=0), pytest.raises(ValueError, match="Unrecognized keyword arguments")), + (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), + (dict(shift=True, axis=0, time_series=np.arange(10)), pytest.raises(ValueError, match="Unrecognized keyword arguments")), + ] + ) + def test_init_conv_kwargs(self, conv_kwargs, expectation): + with expectation: + self.cls(5, mode="conv", window_size=200, **conv_kwargs) + @pytest.mark.parametrize( "mode, ws, expectation", [ @@ -1144,6 +1161,24 @@ def test_init_mode(self, mode, expectation): with expectation: self.cls(5, mode=mode, window_size=window_size) + @pytest.mark.parametrize( + "conv_kwargs, expectation", + [ + (dict(), does_not_raise()), + (dict(axis=0), does_not_raise()), + (dict(axis=1), pytest.raises(ValueError, match="Invalid `axis=1` provided")), + (dict(shift=True), does_not_raise()), + (dict(shift=True, axis=0), does_not_raise()), + (dict(shifts=True, axis=0), pytest.raises(ValueError, match="Unrecognized keyword arguments")), + (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), + (dict(shift=True, axis=0, time_series=np.arange(10)), + pytest.raises(ValueError, match="Unrecognized keyword arguments")), + ] + ) + def test_init_conv_kwargs(self, conv_kwargs, expectation): + with expectation: + self.cls(5, mode="conv", window_size=200, **conv_kwargs) + @pytest.mark.parametrize( "mode, ws, expectation", [ @@ -1778,6 +1813,24 @@ def test_init_mode(self, mode, expectation): with expectation: self.cls(5, mode=mode, window_size=window_size) + @pytest.mark.parametrize( + "conv_kwargs, expectation", + [ + (dict(), does_not_raise()), + (dict(axis=0), does_not_raise()), + (dict(axis=1), pytest.raises(ValueError, match="Invalid `axis=1` provided")), + (dict(shift=True), does_not_raise()), + (dict(shift=True, axis=0), does_not_raise()), + (dict(shifts=True, axis=0), pytest.raises(ValueError, match="Unrecognized keyword arguments")), + (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), + (dict(shift=True, axis=0, time_series=np.arange(10)), + pytest.raises(ValueError, match="Unrecognized keyword arguments")), + ] + ) + def test_init_conv_kwargs(self, conv_kwargs, expectation): + with expectation: + self.cls(5, mode="conv", window_size=200, **conv_kwargs) + @pytest.mark.parametrize( "mode, ws, expectation", [ @@ -2490,6 +2543,24 @@ def test_init_mode(self, mode, expectation): with expectation: self.cls(5, mode=mode, window_size=window_size, decay_rates=np.arange(1, 6)) + @pytest.mark.parametrize( + "conv_kwargs, expectation", + [ + (dict(), does_not_raise()), + (dict(axis=0), does_not_raise()), + (dict(axis=1), pytest.raises(ValueError, match="Invalid `axis=1` provided")), + (dict(shift=True), does_not_raise()), + (dict(shift=True, axis=0), does_not_raise()), + (dict(shifts=True, axis=0), pytest.raises(ValueError, match="Unrecognized keyword arguments")), + (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), + (dict(shift=True, axis=0, time_series=np.arange(10)), + pytest.raises(ValueError, match="Unrecognized keyword arguments")), + ] + ) + def test_init_conv_kwargs(self, conv_kwargs, expectation): + with expectation: + self.cls(5, mode="conv", window_size=200, decay_rates=np.arange(1, 6), **conv_kwargs) + @pytest.mark.parametrize( "mode, ws, expectation", [ @@ -3078,6 +3149,24 @@ def test_init_mode(self, mode, expectation): with expectation: self.cls(5, mode=mode, window_size=window_size) + @pytest.mark.parametrize( + "conv_kwargs, expectation", + [ + (dict(), does_not_raise()), + (dict(axis=0), does_not_raise()), + (dict(axis=1), pytest.raises(ValueError, match="Invalid `axis=1` provided")), + (dict(shift=True), does_not_raise()), + (dict(shift=True, axis=0), does_not_raise()), + (dict(shifts=True, axis=0), pytest.raises(ValueError, match="Unrecognized keyword arguments")), + (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), + (dict(shift=True, axis=0, time_series=np.arange(10)), + pytest.raises(ValueError, match="Unrecognized keyword arguments")), + ] + ) + def test_init_conv_kwargs(self, conv_kwargs, expectation): + with expectation: + self.cls(5, mode="conv", window_size=200, **conv_kwargs) + @pytest.mark.parametrize( "mode, ws, expectation", [ From 9c04c912c4cf4960374384f6e082f54266e6b207 Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Wed, 23 Oct 2024 10:09:22 -0400 Subject: [PATCH 02/14] improved err message --- src/nemos/basis.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index ee6d5a20..b5116632 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -514,7 +514,7 @@ def __init__( # check on convolution kwargs content if "axis" in kwargs and kwargs["axis"] != 0: raise ValueError( - f"Invalid `axis={kwargs['axis']}` provided. This basis requires the " + f"Invalid `axis={kwargs['axis']}` provided. Basis requires the " f"convolution to be applied along the first axis (`axis=0`).\n" "Please transpose your input so that the desired axis for " "convolution is the first dimension (axis=0)." From ee8291b7472aa2f4f4deb3bec92cf30873f73cf2 Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Wed, 23 Oct 2024 10:10:26 -0400 Subject: [PATCH 03/14] linted --- src/nemos/basis.py | 5 +- tests/test_basis.py | 127 +++++++++++++++++++++++++++++++++----------- 2 files changed, 99 insertions(+), 33 deletions(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index b5116632..796a8f3c 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -473,7 +473,7 @@ class Basis(Base, abc.ABC): **kwargs : Only used in "conv" mode. Additional keyword arguments that are passed to `nemos.convolve.create_convolutional_predictor` - + Raises ------ ValueError: @@ -521,7 +521,8 @@ def __init__( ) convolve_params = inspect.signature(create_convolutional_predictor).parameters convolve_configs = { - key for key, param in convolve_params.items() + key + for key, param in convolve_params.items() if param.default is not inspect.Parameter.empty } if not set(kwargs.keys()).issubset(convolve_configs): diff --git a/tests/test_basis.py b/tests/test_basis.py index dfb23c53..a584468e 100644 --- a/tests/test_basis.py +++ b/tests/test_basis.py @@ -500,13 +500,22 @@ def test_init_mode(self, mode, expectation): [ (dict(), does_not_raise()), (dict(axis=0), does_not_raise()), - (dict(axis=1), pytest.raises(ValueError, match="Invalid `axis=1` provided")), + ( + dict(axis=1), + pytest.raises(ValueError, match="Invalid `axis=1` provided"), + ), (dict(shift=True), does_not_raise()), (dict(shift=True, axis=0), does_not_raise()), - (dict(shifts=True, axis=0), pytest.raises(ValueError, match="Unrecognized keyword arguments")), + ( + dict(shifts=True, axis=0), + pytest.raises(ValueError, match="Unrecognized keyword arguments"), + ), (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), - (dict(shift=True, axis=0, time_series=np.arange(10)), pytest.raises(ValueError, match="Unrecognized keyword arguments")), - ] + ( + dict(shift=True, axis=0, time_series=np.arange(10)), + pytest.raises(ValueError, match="Unrecognized keyword arguments"), + ), + ], ) def test_init_conv_kwargs(self, conv_kwargs, expectation): with expectation: @@ -584,7 +593,10 @@ def test_set_params( for i in range(len(pars)): for j in range(i + 1, len(pars)): - with pytest.raises(AttributeError, match="can't set attribute 'mode'|property 'mode' of "): + with pytest.raises( + AttributeError, + match="can't set attribute 'mode'|property 'mode' of ", + ): par_set = { keys[i]: pars[keys[i]], keys[j]: pars[keys[j]], @@ -1166,14 +1178,22 @@ def test_init_mode(self, mode, expectation): [ (dict(), does_not_raise()), (dict(axis=0), does_not_raise()), - (dict(axis=1), pytest.raises(ValueError, match="Invalid `axis=1` provided")), + ( + dict(axis=1), + pytest.raises(ValueError, match="Invalid `axis=1` provided"), + ), (dict(shift=True), does_not_raise()), (dict(shift=True, axis=0), does_not_raise()), - (dict(shifts=True, axis=0), pytest.raises(ValueError, match="Unrecognized keyword arguments")), + ( + dict(shifts=True, axis=0), + pytest.raises(ValueError, match="Unrecognized keyword arguments"), + ), (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), - (dict(shift=True, axis=0, time_series=np.arange(10)), - pytest.raises(ValueError, match="Unrecognized keyword arguments")), - ] + ( + dict(shift=True, axis=0, time_series=np.arange(10)), + pytest.raises(ValueError, match="Unrecognized keyword arguments"), + ), + ], ) def test_init_conv_kwargs(self, conv_kwargs, expectation): with expectation: @@ -1237,7 +1257,10 @@ def test_set_params( for i in range(len(pars)): for j in range(i + 1, len(pars)): - with pytest.raises(AttributeError, match="can't set attribute 'mode'|property 'mode' of "): + with pytest.raises( + AttributeError, + match="can't set attribute 'mode'|property 'mode' of ", + ): par_set = { keys[i]: pars[keys[i]], keys[j]: pars[keys[j]], @@ -1818,14 +1841,22 @@ def test_init_mode(self, mode, expectation): [ (dict(), does_not_raise()), (dict(axis=0), does_not_raise()), - (dict(axis=1), pytest.raises(ValueError, match="Invalid `axis=1` provided")), + ( + dict(axis=1), + pytest.raises(ValueError, match="Invalid `axis=1` provided"), + ), (dict(shift=True), does_not_raise()), (dict(shift=True, axis=0), does_not_raise()), - (dict(shifts=True, axis=0), pytest.raises(ValueError, match="Unrecognized keyword arguments")), + ( + dict(shifts=True, axis=0), + pytest.raises(ValueError, match="Unrecognized keyword arguments"), + ), (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), - (dict(shift=True, axis=0, time_series=np.arange(10)), - pytest.raises(ValueError, match="Unrecognized keyword arguments")), - ] + ( + dict(shift=True, axis=0, time_series=np.arange(10)), + pytest.raises(ValueError, match="Unrecognized keyword arguments"), + ), + ], ) def test_init_conv_kwargs(self, conv_kwargs, expectation): with expectation: @@ -1889,7 +1920,10 @@ def test_set_params( for i in range(len(pars)): for j in range(i + 1, len(pars)): - with pytest.raises(AttributeError, match="can't set attribute 'mode'|property 'mode' of "): + with pytest.raises( + AttributeError, + match="can't set attribute 'mode'|property 'mode' of ", + ): par_set = { keys[i]: pars[keys[i]], keys[j]: pars[keys[j]], @@ -2548,18 +2582,32 @@ def test_init_mode(self, mode, expectation): [ (dict(), does_not_raise()), (dict(axis=0), does_not_raise()), - (dict(axis=1), pytest.raises(ValueError, match="Invalid `axis=1` provided")), + ( + dict(axis=1), + pytest.raises(ValueError, match="Invalid `axis=1` provided"), + ), (dict(shift=True), does_not_raise()), (dict(shift=True, axis=0), does_not_raise()), - (dict(shifts=True, axis=0), pytest.raises(ValueError, match="Unrecognized keyword arguments")), + ( + dict(shifts=True, axis=0), + pytest.raises(ValueError, match="Unrecognized keyword arguments"), + ), (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), - (dict(shift=True, axis=0, time_series=np.arange(10)), - pytest.raises(ValueError, match="Unrecognized keyword arguments")), - ] + ( + dict(shift=True, axis=0, time_series=np.arange(10)), + pytest.raises(ValueError, match="Unrecognized keyword arguments"), + ), + ], ) def test_init_conv_kwargs(self, conv_kwargs, expectation): with expectation: - self.cls(5, mode="conv", window_size=200, decay_rates=np.arange(1, 6), **conv_kwargs) + self.cls( + 5, + mode="conv", + window_size=200, + decay_rates=np.arange(1, 6), + **conv_kwargs, + ) @pytest.mark.parametrize( "mode, ws, expectation", @@ -2628,7 +2676,10 @@ def test_set_params( for i in range(len(pars)): for j in range(i + 1, len(pars)): - with pytest.raises(AttributeError, match="can't set attribute 'mode'|property 'mode' of "): + with pytest.raises( + AttributeError, + match="can't set attribute 'mode'|property 'mode' of ", + ): par_set = { keys[i]: pars[keys[i]], keys[j]: pars[keys[j]], @@ -3154,14 +3205,22 @@ def test_init_mode(self, mode, expectation): [ (dict(), does_not_raise()), (dict(axis=0), does_not_raise()), - (dict(axis=1), pytest.raises(ValueError, match="Invalid `axis=1` provided")), + ( + dict(axis=1), + pytest.raises(ValueError, match="Invalid `axis=1` provided"), + ), (dict(shift=True), does_not_raise()), (dict(shift=True, axis=0), does_not_raise()), - (dict(shifts=True, axis=0), pytest.raises(ValueError, match="Unrecognized keyword arguments")), + ( + dict(shifts=True, axis=0), + pytest.raises(ValueError, match="Unrecognized keyword arguments"), + ), (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), - (dict(shift=True, axis=0, time_series=np.arange(10)), - pytest.raises(ValueError, match="Unrecognized keyword arguments")), - ] + ( + dict(shift=True, axis=0, time_series=np.arange(10)), + pytest.raises(ValueError, match="Unrecognized keyword arguments"), + ), + ], ) def test_init_conv_kwargs(self, conv_kwargs, expectation): with expectation: @@ -3225,7 +3284,10 @@ def test_set_params( for i in range(len(pars)): for j in range(i + 1, len(pars)): - with pytest.raises(AttributeError, match="can't set attribute 'mode'|property 'mode' of "): + with pytest.raises( + AttributeError, + match="can't set attribute 'mode'|property 'mode' of ", + ): par_set = { keys[i]: pars[keys[i]], keys[j]: pars[keys[j]], @@ -3881,7 +3943,10 @@ def test_set_params( for i in range(len(pars)): for j in range(i + 1, len(pars)): - with pytest.raises(AttributeError, match="can't set attribute 'mode'|property 'mode' of "): + with pytest.raises( + AttributeError, + match="can't set attribute 'mode'|property 'mode' of ", + ): par_set = { keys[i]: pars[keys[i]], keys[j]: pars[keys[j]], From b980f12d773b6a1b1bec8e9f7788756cc6a7f4a5 Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Wed, 23 Oct 2024 10:13:42 -0400 Subject: [PATCH 04/14] improved error message logic --- src/nemos/basis.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index 796a8f3c..cf60d079 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -525,8 +525,11 @@ def __init__( for key, param in convolve_params.items() if param.default is not inspect.Parameter.empty } + # do not encourage to set axis. + convolve_configs = convolve_configs.difference({"axis"}) if not set(kwargs.keys()).issubset(convolve_configs): - invalid = set(kwargs.keys()).difference(convolve_configs) + # remove axis in case axis=0, was passed which is allowed. + invalid = set(kwargs.keys()).difference(convolve_configs).difference({"axis"}) raise ValueError( f"Unrecognized keyword arguments: {invalid}. " f"Allowed convolution keyword arguments are: {convolve_configs}." From 040397bd0eaae7ab122711c264e526504d33407e Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Wed, 23 Oct 2024 10:14:37 -0400 Subject: [PATCH 05/14] linted again --- src/nemos/basis.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index cf60d079..e1770357 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -529,7 +529,9 @@ def __init__( convolve_configs = convolve_configs.difference({"axis"}) if not set(kwargs.keys()).issubset(convolve_configs): # remove axis in case axis=0, was passed which is allowed. - invalid = set(kwargs.keys()).difference(convolve_configs).difference({"axis"}) + invalid = ( + set(kwargs.keys()).difference(convolve_configs).difference({"axis"}) + ) raise ValueError( f"Unrecognized keyword arguments: {invalid}. " f"Allowed convolution keyword arguments are: {convolve_configs}." From 706279ba302d08959a6503d24fda8cc49c9e650c Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Wed, 23 Oct 2024 10:25:42 -0400 Subject: [PATCH 06/14] improved docstrings --- src/nemos/basis.py | 49 +++++++++++++++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index e1770357..9574bdf4 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -471,8 +471,11 @@ class Basis(Base, abc.ABC): minimum and the maximum of the samples provided when evaluating the basis. If a sample is outside the bounds, the basis will return NaN. **kwargs : - Only used in "conv" mode. Additional keyword arguments that are passed to - `nemos.convolve.create_convolutional_predictor` + Additional keyword arguments passed to `nemos.convolve.create_convolutional_predictor` when + `mode='conv'`; These arguments are used to change the default behavior of the convolution. + For example, changing the `predictor_causality`, which by default is set to `"causal"`. + Note that one cannot change the default value for the `axis` parameter. Basis assumes + that the convolution axis is `axis=0`. Raises ------ @@ -1471,8 +1474,11 @@ class MSplineBasis(SplineBasis): minimum and the maximum of the samples provided when evaluating the basis. If a sample is outside the bounds, the basis will return NaN. **kwargs: - Only used in "conv" mode. Additional keyword arguments that are passed to - `nemos.convolve.create_convolutional_predictor` + Additional keyword arguments passed to `nemos.convolve.create_convolutional_predictor` when + `mode='conv'`; These arguments are used to change the default behavior of the convolution. + For example, changing the `predictor_causality`, which by default is set to `"causal"`. + Note that one cannot change the default value for the `axis` parameter. Basis assumes + that the convolution axis is `axis=0`. Examples -------- @@ -1629,8 +1635,11 @@ class BSplineBasis(SplineBasis): minimum and the maximum of the samples provided when evaluating the basis. If a sample is outside the bounds, the basis will return NaN. **kwargs : - Only used in "conv" mode. Additional keyword arguments that are passed to - `nemos.convolve.create_convolutional_predictor` + Additional keyword arguments passed to `nemos.convolve.create_convolutional_predictor` when + `mode='conv'`; These arguments are used to change the default behavior of the convolution. + For example, changing the `predictor_causality`, which by default is set to `"causal"`. + Note that one cannot change the default value for the `axis` parameter. Basis assumes + that the convolution axis is `axis=0`. Attributes ---------- @@ -1748,8 +1757,11 @@ class CyclicBSplineBasis(SplineBasis): minimum and the maximum of the samples provided when evaluating the basis. If a sample is outside the bounds, the basis will return NaN. **kwargs : - Only used in "conv" mode. Additional keyword arguments that are passed to - `nemos.convolve.create_convolutional_predictor` + Additional keyword arguments passed to `nemos.convolve.create_convolutional_predictor` when + `mode='conv'`; These arguments are used to change the default behavior of the convolution. + For example, changing the `predictor_causality`, which by default is set to `"causal"`. + Note that one cannot change the default value for the `axis` parameter. Basis assumes + that the convolution axis is `axis=0`. Attributes ---------- @@ -1890,8 +1902,11 @@ class RaisedCosineBasisLinear(Basis): minimum and the maximum of the samples provided when evaluating the basis. If a sample is outside the bounds, the basis will return NaN. **kwargs : - Only used in "conv" mode. Additional keyword arguments that are passed to - `nemos.convolve.create_convolutional_predictor` + Additional keyword arguments passed to `nemos.convolve.create_convolutional_predictor` when + `mode='conv'`; These arguments are used to change the default behavior of the convolution. + For example, changing the `predictor_causality`, which by default is set to `"causal"`. + Note that one cannot change the default value for the `axis` parameter. Basis assumes + that the convolution axis is `axis=0`. # References ------------ @@ -2083,8 +2098,11 @@ class RaisedCosineBasisLog(RaisedCosineBasisLinear): minimum and the maximum of the samples provided when evaluating the basis. If a sample is outside the bounds, the basis will return NaN. **kwargs : - Only used in "conv" mode. Additional keyword arguments that are passed to - `nemos.convolve.create_convolutional_predictor` + Additional keyword arguments passed to `nemos.convolve.create_convolutional_predictor` when + `mode='conv'`; These arguments are used to change the default behavior of the convolution. + For example, changing the `predictor_causality`, which by default is set to `"causal"`. + Note that one cannot change the default value for the `axis` parameter. Basis assumes + that the convolution axis is `axis=0`. # References ------------ @@ -2237,8 +2255,11 @@ class OrthExponentialBasis(Basis): minimum and the maximum of the samples provided when evaluating the basis. If a sample is outside the bounds, the basis will return NaN. **kwargs : - Only used in "conv" mode. Additional keyword arguments that are passed to - `nemos.convolve.create_convolutional_predictor` + Additional keyword arguments passed to `nemos.convolve.create_convolutional_predictor` when + `mode='conv'`; These arguments are used to change the default behavior of the convolution. + For example, changing the `predictor_causality`, which by default is set to `"causal"`. + Note that one cannot change the default value for the `axis` parameter. Basis assumes + that the convolution axis is `axis=0`. """ def __init__( From 8426fc6783f57a3ed774767b52e1875f41819437 Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Wed, 23 Oct 2024 10:28:56 -0400 Subject: [PATCH 07/14] fixed docstrings --- src/nemos/basis.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index 9574bdf4..12231bce 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -1323,8 +1323,11 @@ class SplineBasis(Basis, abc.ABC): minimum and the maximum of the samples provided when evaluating the basis. If a sample is outside the bounds, the basis will return NaN. **kwargs : - Only used in "conv" mode. Additional keyword arguments that are passed to - `nemos.convolve.create_convolutional_predictor` + Additional keyword arguments passed to `nemos.convolve.create_convolutional_predictor` when + `mode='conv'`; These arguments are used to change the default behavior of the convolution. + For example, changing the `predictor_causality`, which by default is set to `"causal"`. + Note that one cannot change the default value for the `axis` parameter. Basis assumes + that the convolution axis is `axis=0`. Attributes ---------- From 861aeb5b2db2c6092094dff2271770d0fa4e5f4a Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Wed, 23 Oct 2024 10:52:12 -0400 Subject: [PATCH 08/14] bugfix check --- src/nemos/basis.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index 12231bce..32e04701 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -528,9 +528,9 @@ def __init__( for key, param in convolve_params.items() if param.default is not inspect.Parameter.empty } - # do not encourage to set axis. - convolve_configs = convolve_configs.difference({"axis"}) if not set(kwargs.keys()).issubset(convolve_configs): + # do not encourage to set axis. + convolve_configs = convolve_configs.difference({"axis"}) # remove axis in case axis=0, was passed which is allowed. invalid = ( set(kwargs.keys()).difference(convolve_configs).difference({"axis"}) From e19fe2621d3c068563718f11d01df10ab3d05b08 Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Wed, 23 Oct 2024 11:42:48 -0400 Subject: [PATCH 09/14] improved modularity --- src/nemos/basis.py | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index 32e04701..e152f063 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -509,15 +509,31 @@ def __init__( self.window_size = window_size self.bounds = bounds - if mode == "eval" and kwargs: + self._check_convolution_kwargs() + + self.kernel_ = None + self._identifiability_constraints = False + + def _check_convolution_kwargs(self): + """Check convolution kwargs settings. + + Raises + ------ + ValueError: + - If `self._conv_kwargs` are not None and `mode =="eval". + - If `axis` is provided as an argument, and it is different from 0 + (samples must always be in the first axis). + - If `self._conv_kwargs` include parameters not recognized or that do not have + default values in `create_convolutional_predictor`. + """ + if self._mode == "eval" and self._conv_kwargs: raise ValueError( - f"kwargs should only be set when mode=='conv', but '{mode}' provided instead!" + f"kwargs should only be set when mode=='conv', but '{self._mode}' provided instead!" ) - # check on convolution kwargs content - if "axis" in kwargs and kwargs["axis"] != 0: + if "axis" in self._conv_kwargs and self._conv_kwargs["axis"] != 0: raise ValueError( - f"Invalid `axis={kwargs['axis']}` provided. Basis requires the " + f"Invalid `axis={self._conv_kwargs['axis']}` provided. Basis requires the " f"convolution to be applied along the first axis (`axis=0`).\n" "Please transpose your input so that the desired axis for " "convolution is the first dimension (axis=0)." @@ -526,23 +542,21 @@ def __init__( convolve_configs = { key for key, param in convolve_params.items() - if param.default is not inspect.Parameter.empty + if param.default is not inspect.Parameter.empty # prevent user from passing directly + # `basis_matrix` or `time_series` in kwargs. } - if not set(kwargs.keys()).issubset(convolve_configs): + if not set(self._conv_kwargs.keys()).issubset(convolve_configs): # do not encourage to set axis. convolve_configs = convolve_configs.difference({"axis"}) - # remove axis in case axis=0, was passed which is allowed. + # remove the parameter in case axis=0 was passed, since it is allowed. invalid = ( - set(kwargs.keys()).difference(convolve_configs).difference({"axis"}) + set(self._conv_kwargs.keys()).difference(convolve_configs).difference({"axis"}) ) raise ValueError( f"Unrecognized keyword arguments: {invalid}. " f"Allowed convolution keyword arguments are: {convolve_configs}." ) - self.kernel_ = None - self._identifiability_constraints = False - @property def n_basis_funcs(self): return self._n_basis_funcs From 0928644041449a079a673adc654149e7f9b2181a Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Wed, 23 Oct 2024 11:48:51 -0400 Subject: [PATCH 10/14] moved checks to dedicated function --- src/nemos/basis.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index e152f063..3d4d1d49 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -542,15 +542,18 @@ def _check_convolution_kwargs(self): convolve_configs = { key for key, param in convolve_params.items() - if param.default is not inspect.Parameter.empty # prevent user from passing directly - # `basis_matrix` or `time_series` in kwargs. + if param.default + is not inspect.Parameter.empty # prevent user from passing directly + # `basis_matrix` or `time_series` in kwargs. } if not set(self._conv_kwargs.keys()).issubset(convolve_configs): # do not encourage to set axis. convolve_configs = convolve_configs.difference({"axis"}) # remove the parameter in case axis=0 was passed, since it is allowed. invalid = ( - set(self._conv_kwargs.keys()).difference(convolve_configs).difference({"axis"}) + set(self._conv_kwargs.keys()) + .difference(convolve_configs) + .difference({"axis"}) ) raise ValueError( f"Unrecognized keyword arguments: {invalid}. " From 4aed9dfa8083822149f1e52d905c6fe97730c3d9 Mon Sep 17 00:00:00 2001 From: Edoardo Balzani Date: Fri, 25 Oct 2024 09:49:57 -0400 Subject: [PATCH 11/14] Update src/nemos/basis.py Co-authored-by: William F. Broderick --- src/nemos/basis.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index 3d4d1d49..49260460 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -543,8 +543,9 @@ def _check_convolution_kwargs(self): key for key, param in convolve_params.items() if param.default - is not inspect.Parameter.empty # prevent user from passing directly + # prevent user from passing # `basis_matrix` or `time_series` in kwargs. + is not inspect.Parameter.empty } if not set(self._conv_kwargs.keys()).issubset(convolve_configs): # do not encourage to set axis. From febc545b961b4218f71108ac1acfbf7e3cacc170 Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Fri, 25 Oct 2024 10:01:06 -0400 Subject: [PATCH 12/14] linted test and changed to not allow setting axis --- src/nemos/basis.py | 4 +- tests/test_basis.py | 120 +++++++++++++++++++++++++++++++++----------- 2 files changed, 92 insertions(+), 32 deletions(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index 3d4d1d49..774c8e7a 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -531,9 +531,9 @@ def _check_convolution_kwargs(self): f"kwargs should only be set when mode=='conv', but '{self._mode}' provided instead!" ) - if "axis" in self._conv_kwargs and self._conv_kwargs["axis"] != 0: + if "axis" in self._conv_kwargs: raise ValueError( - f"Invalid `axis={self._conv_kwargs['axis']}` provided. Basis requires the " + f"Setting the `axis` parameter is not allowed. Basis requires the " f"convolution to be applied along the first axis (`axis=0`).\n" "Please transpose your input so that the desired axis for " "convolution is the first dimension (axis=0)." diff --git a/tests/test_basis.py b/tests/test_basis.py index a584468e..fe10c43b 100644 --- a/tests/test_basis.py +++ b/tests/test_basis.py @@ -499,20 +499,32 @@ def test_init_mode(self, mode, expectation): "conv_kwargs, expectation", [ (dict(), does_not_raise()), - (dict(axis=0), does_not_raise()), + ( + dict(axis=0), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), + ), ( dict(axis=1), - pytest.raises(ValueError, match="Invalid `axis=1` provided"), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), ), (dict(shift=True), does_not_raise()), - (dict(shift=True, axis=0), does_not_raise()), ( - dict(shifts=True, axis=0), + dict(shift=True, axis=0), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), + ), + ( + dict(shifts=True), pytest.raises(ValueError, match="Unrecognized keyword arguments"), ), - (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), + (dict(shift=True, predictor_causality="causal"), does_not_raise()), ( - dict(shift=True, axis=0, time_series=np.arange(10)), + dict(shift=True, time_series=np.arange(10)), pytest.raises(ValueError, match="Unrecognized keyword arguments"), ), ], @@ -1177,20 +1189,32 @@ def test_init_mode(self, mode, expectation): "conv_kwargs, expectation", [ (dict(), does_not_raise()), - (dict(axis=0), does_not_raise()), + ( + dict(axis=0), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), + ), ( dict(axis=1), - pytest.raises(ValueError, match="Invalid `axis=1` provided"), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), ), (dict(shift=True), does_not_raise()), - (dict(shift=True, axis=0), does_not_raise()), ( - dict(shifts=True, axis=0), + dict(shift=True, axis=0), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), + ), + ( + dict(shifts=True), pytest.raises(ValueError, match="Unrecognized keyword arguments"), ), - (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), + (dict(shift=True, predictor_causality="causal"), does_not_raise()), ( - dict(shift=True, axis=0, time_series=np.arange(10)), + dict(shift=True, time_series=np.arange(10)), pytest.raises(ValueError, match="Unrecognized keyword arguments"), ), ], @@ -1840,20 +1864,32 @@ def test_init_mode(self, mode, expectation): "conv_kwargs, expectation", [ (dict(), does_not_raise()), - (dict(axis=0), does_not_raise()), + ( + dict(axis=0), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), + ), ( dict(axis=1), - pytest.raises(ValueError, match="Invalid `axis=1` provided"), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), ), (dict(shift=True), does_not_raise()), - (dict(shift=True, axis=0), does_not_raise()), ( - dict(shifts=True, axis=0), + dict(shift=True, axis=0), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), + ), + ( + dict(shifts=True), pytest.raises(ValueError, match="Unrecognized keyword arguments"), ), - (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), + (dict(shift=True, predictor_causality="causal"), does_not_raise()), ( - dict(shift=True, axis=0, time_series=np.arange(10)), + dict(shift=True, time_series=np.arange(10)), pytest.raises(ValueError, match="Unrecognized keyword arguments"), ), ], @@ -2581,20 +2617,32 @@ def test_init_mode(self, mode, expectation): "conv_kwargs, expectation", [ (dict(), does_not_raise()), - (dict(axis=0), does_not_raise()), + ( + dict(axis=0), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), + ), ( dict(axis=1), - pytest.raises(ValueError, match="Invalid `axis=1` provided"), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), ), (dict(shift=True), does_not_raise()), - (dict(shift=True, axis=0), does_not_raise()), ( - dict(shifts=True, axis=0), + dict(shift=True, axis=0), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), + ), + ( + dict(shifts=True), pytest.raises(ValueError, match="Unrecognized keyword arguments"), ), - (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), + (dict(shift=True, predictor_causality="causal"), does_not_raise()), ( - dict(shift=True, axis=0, time_series=np.arange(10)), + dict(shift=True, time_series=np.arange(10)), pytest.raises(ValueError, match="Unrecognized keyword arguments"), ), ], @@ -3204,20 +3252,32 @@ def test_init_mode(self, mode, expectation): "conv_kwargs, expectation", [ (dict(), does_not_raise()), - (dict(axis=0), does_not_raise()), + ( + dict(axis=0), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), + ), ( dict(axis=1), - pytest.raises(ValueError, match="Invalid `axis=1` provided"), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), ), (dict(shift=True), does_not_raise()), - (dict(shift=True, axis=0), does_not_raise()), ( - dict(shifts=True, axis=0), + dict(shift=True, axis=0), + pytest.raises( + ValueError, match="Setting the `axis` parameter is not allowed" + ), + ), + ( + dict(shifts=True), pytest.raises(ValueError, match="Unrecognized keyword arguments"), ), - (dict(shift=True, axis=0, predictor_causality="causal"), does_not_raise()), + (dict(shift=True, predictor_causality="causal"), does_not_raise()), ( - dict(shift=True, axis=0, time_series=np.arange(10)), + dict(shift=True, time_series=np.arange(10)), pytest.raises(ValueError, match="Unrecognized keyword arguments"), ), ], From bfe707247ebcdcebd2d74bd17530e6851f181182 Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Fri, 25 Oct 2024 10:12:55 -0400 Subject: [PATCH 13/14] linted test and changed to not allow setting axis --- src/nemos/basis.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index f34182f6..45000702 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -545,7 +545,7 @@ def _check_convolution_kwargs(self): if param.default # prevent user from passing # `basis_matrix` or `time_series` in kwargs. - is not inspect.Parameter.empty + is not inspect.Parameter.empty } if not set(self._conv_kwargs.keys()).issubset(convolve_configs): # do not encourage to set axis. From d51f8d34f4bc97d4ad1aaa976f8734e65bf628d1 Mon Sep 17 00:00:00 2001 From: BalzaniEdoardo Date: Fri, 25 Oct 2024 10:15:45 -0400 Subject: [PATCH 14/14] flake8 fix --- src/nemos/basis.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nemos/basis.py b/src/nemos/basis.py index 45000702..f0c988ad 100644 --- a/src/nemos/basis.py +++ b/src/nemos/basis.py @@ -533,8 +533,8 @@ def _check_convolution_kwargs(self): if "axis" in self._conv_kwargs: raise ValueError( - f"Setting the `axis` parameter is not allowed. Basis requires the " - f"convolution to be applied along the first axis (`axis=0`).\n" + "Setting the `axis` parameter is not allowed. Basis requires the " + "convolution to be applied along the first axis (`axis=0`).\n" "Please transpose your input so that the desired axis for " "convolution is the first dimension (axis=0)." )