Fourier #300

BalzaniEdoardo · 2025-01-23T21:44:32Z

Add a Fourier basis.

This PR adds a Fourier basis. The API is as follows:

import nemos as nmo
import numpy as np
import matplotlib.pyplot as plt

# this configuration exclude the 0 frequency creating 2*n_frequencies bases.
bas = nmo.basis.FourierEval(n_frequencies=5)
out = bas.compute_features(np.linspace(0, 1, 200))

plt.figure()
plt.title("No 0 frequency")
plt.plot(out)
plt.show()

# this configuration exclude the 0 frequency creating 2*n_frequencies bases + 1.
bas = nmo.basis.FourierEval(n_frequencies=5, include_constant=True)
out = bas.compute_features(np.linspace(0, 1, 200))

plt.figure()
plt.title("Include 0 frequency")
plt.plot(out)
plt.show()

billbrod

Just some minor requested changes about the docstrings and tests.

Looks like a lot of minor changes in the basis tests -- is that to make sure that the window size is big enough for the Fourier Conv basis?
Also some line-wrapping -- did the version of black change or something?
I think we should add tests that check the relationship with FFT.

billbrod · 2025-06-23T16:43:35Z

docs/background/basis/README.md

+     - 🔵 Conv
+
+   * - **Fourier Basis**
+     - .. plot:: scripts/basis_figs.py plot_fourier_basis


this image looks like a mess -- is there a more informative way to plot it?

I think plotting fewer frequencies would help

billbrod · 2025-06-23T16:44:09Z

src/nemos/basis/_fourier_basis.py

+    n_frequencies :
+        The number of frequencies for the Fourier basis.
+    include_constant:
+        Include the constant term, which corresponds to 0 frequency. Default is False.


Suggested change

Include the constant term, which corresponds to 0 frequency. Default is False.

Include the constant term, which corresponds to 0 frequency.

don't describe defaults in docstring -- it's already in signature

billbrod · 2025-06-23T16:44:42Z

src/nemos/basis/_fourier_basis.py

+        self,
+        n_frequencies: int,
+        include_constant: bool = False,
+        phase_sign: int = 1,


not described in docstring

billbrod · 2025-06-23T16:45:02Z

src/nemos/basis/_fourier_basis.py

+
+    @property
+    def phase_sign(self):
+        """Read-only phase sign property."""


Suggested change

"""Read-only phase sign property."""

doesn't add anything.

(so either remove, or make it more descriptive and add some for the other properties as well)

billbrod · 2025-06-23T16:52:59Z

src/nemos/basis/basis.py

+        By default, ``phase_sign = 1``, which corresponds to the standard orthonormal Fourier basis.
+        Setting ``phase_sign = -1`` inverts the sign of the phase angle and aligns the basis
+        with the Discrete Fourier Transform (DFT) convention. With such convention the convolution
+        with this basis yields the FFT time evolution.


"FFT time evolution"? is that the right word?

And do we have a test for this?

billbrod · 2025-06-23T16:54:58Z

src/nemos/basis/basis.py

+                \text{phase_sign} \cdot \sin(2\pi k x), & \text{otherwise}
+            \end{cases}
+
+        By default, ``phase_sign = -1``, which aligns the basis with the Discrete Fourier Transform (DFT)


remove "default" and should probably be identical to the description in FourierEval

billbrod · 2025-06-23T16:55:08Z

src/nemos/basis/basis.py

+                \text{phase_sign} \cdot \sin(2\pi k x), & \text{otherwise}
+            \end{cases}
+
+        By default, ``phase_sign = 1``, which corresponds to the standard orthonormal Fourier basis.


remove "default"

billbrod · 2025-06-23T16:55:37Z

src/nemos/basis/basis.py

+    conv_kwargs:
+        Additional keyword arguments passed to :func:`nemos.convolve.create_convolutional_predictor`;
+        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"``.


again, remove default

and should probably have an example of how to do this in doctest

billbrod · 2025-06-23T16:58:42Z

tests/test_basis.py

@@ -2607,6 +2670,106 @@ def test_samples_range_matches_compute_features_requirements(
        basis_obj.compute_features(np.linspace(*sample_range, 100))


+class TestFourierBasis(BasisFuncsTesting):


should test window size is too small for Nyquist

billbrod · 2025-06-23T16:59:53Z

tests/test_transformer_basis.py

+        trans_bas_b.n_frequencies = 3
+        assert bas_b.n_frequencies == 5


what's going on here?

as in, why does it the transformer basis have more frequencies than the non-transformer one?

sjvenditto

Looking great! I have a few comments on missing test cases, and I think the Fourier basis docstring could use a little more information

sjvenditto · 2025-06-24T14:32:12Z

docs/background/basis/README.md

+     - 🔵 Conv
+
+   * - **Fourier Basis**
+     - .. plot:: scripts/basis_figs.py plot_fourier_basis


I think plotting fewer frequencies would help

sjvenditto · 2025-06-24T19:14:21Z

src/nemos/basis/_fourier_basis.py

+        n_frequencies: int,
+        include_constant: bool = False,
+        phase_sign: int = 1,
+        label: Optional[str] = "RaisedCosineBasisLinear",


Suggested change

label: Optional[str] = "RaisedCosineBasisLinear",

label: Optional[str] = "Fourier",

sjvenditto · 2025-06-24T19:24:33Z

tests/test_basis.py

+    HistoryConv,
+    IdentityEval,
+    TransformerBasis,
+)
 from nemos.basis._basis import (


The tests test_expected_output_eval_on_grid,test_expected_output_compute_features, test_expected_output_split_by_feature are missing cases for Fourier

sjvenditto · 2025-06-24T19:32:42Z

tests/test_basis.py

+                basis.CyclicBSplineConv: "'mylabel': CyclicBSplineConv(n_basis_funcs=5, window_size=11, order=4)",
+                basis.MSplineConv: "'mylabel': MSplineConv(n_basis_funcs=5, window_size=11, order=4)",
+                basis.OrthExponentialConv: "'mylabel': OrthExponentialConv(n_basis_funcs=5, window_size=11)",
+                basis.HistoryConv: "'mylabel': HistoryConv(window_size=11)",


no expected output here for Fourier

sjvenditto · 2025-06-24T19:38:49Z

src/nemos/basis/_fourier_basis.py

+
+
+class FourierBasis(Basis, AtomicBasisMixin, abc.ABC):
+    """Fourier Basis.


Should this give a little more of a description? The other basis functions have more than this

BalzaniEdoardo · 2025-06-25T13:38:47Z

of minor changes in the b

Just some minor requested changes about the docstrings and tests.
* Looks like a lot of minor changes in the basis tests -- is that to make sure that the window size is big enough for the Fourier Conv basis?

Yes, the window size in many of the tests was too small for the nyqst check of the basis, so I had to make sure that the check passed.

* Also some line-wrapping -- did the version of black change or something?

Yes, black was upgraded. I am calling tox -e fix with no changes in the configurations.

* I think we should add tests that check the relationship with FFT.

I will, good call

BalzaniEdoardo · 2025-06-25T18:23:49Z

Add a public property frequencies that returns the frequency value.
Setter will change the n_frequencies and checks for integers values.

…rier

BalzaniEdoardo added 3 commits January 23, 2025 15:18

first commit

a017437

Merge branch 'main' into fourier

c075b9a

added fourier basis

32e90b7

BalzaniEdoardo marked this pull request as draft January 23, 2025 21:44

linted

ef96185

billbrod mentioned this pull request Jan 28, 2025

Add fourier #49

Closed

BalzaniEdoardo added 4 commits January 28, 2025 12:23

Merge branch 'development' into fourier

a8246d8

Merge branch 'main' into fourier

ad5ac6e

merged development

84423f3

fix pydocstyle

8f5587a

BalzaniEdoardo marked this pull request as ready for review June 3, 2025 14:03

BalzaniEdoardo marked this pull request as draft June 3, 2025 16:17

BalzaniEdoardo added 14 commits June 3, 2025 12:18

fix all current tests

3ebd74d

fixed add common tests and added conv basis

ba237b8

add to TestEvalBasis

86745c6

add to TestConvBasis

44c480b

added class specific tests

592c376

fix pydocstyle

2d01549

add plot fourier to plot basis

6eba401

fix transformer tests

652fefc

fixed all tests

459927c

fix test basis window_size allowed

1477f7f

add check for window_size

1fae4a4

fix transformer basis window size

85636e2

add basis to list

6155da1

fix docs rendering

2c8f6f6

BalzaniEdoardo marked this pull request as ready for review June 18, 2025 19:11

BalzaniEdoardo added 3 commits June 19, 2025 16:22

add an env for doctests only

b5fb3ae

fix doctests

2d0422e

fix doctests all

a05dd32

Merge branch 'development' into fourier

5f77595

billbrod approved these changes Jun 23, 2025

View reviewed changes

sjvenditto requested changes Jun 24, 2025

View reviewed changes

BalzaniEdoardo added 2 commits June 25, 2025 14:11

shift angles for conv fourier basis

c5ac8a3

linted test

3751dc6

BalzaniEdoardo and others added 23 commits June 25, 2025 22:53

allow complex bases

0e7f4fd

cleaned up var names

dffc263

make n_basis_funcs settable, and bugfixed setter for include constant

e6e67bd

drop 0 column of imaginary components if basis includes constant

b3f8b0d

make n_basis_funcs settable, and bugfixed setter for include constant

95863e2

bugfix count n basis

aff664c

bugfix count n basis

046cf4d

delete additional columns if the input ndim is > 1

1e9d6d1

add test complex basis

8ae7894

fix masking

c69a1cc

move stuff to conf test

9ccfbd5

merge fix

1686ca5

fix reshape

1a2a588

unfinished bugfix

467507a

fix concatenation of real and imaginary components by reshaping first

12e0fa3

Update _basis_mixin.py

e3ef0e7

merge sarah fix

7a5168c

Merge branch 'fourier' of github.com:flatironinstitute/nemos into fou…

f86e4a8

…rier

split out basis dimension before kron in MultiplicativeBasis

683b66b

bugfix expand complex

49e875f

bugfix set kernel

bffd8d3

fix a test

79a63ee

actual fix on conv

b140aef

	Include the constant term, which corresponds to 0 frequency. Default is False.
	Include the constant term, which corresponds to 0 frequency.

		@@ -2607,6 +2670,106 @@ def test_samples_range_matches_compute_features_requirements(
		basis_obj.compute_features(np.linspace(*sample_range, 100))


		class TestFourierBasis(BasisFuncsTesting):

		trans_bas_b.n_frequencies = 3
		assert bas_b.n_frequencies == 5

	label: Optional[str] = "RaisedCosineBasisLinear",
	label: Optional[str] = "Fourier",



		class FourierBasis(Basis, AtomicBasisMixin, abc.ABC):
		"""Fourier Basis.

Fourier #300

Are you sure you want to change the base?

Fourier #300

Uh oh!

Conversation

BalzaniEdoardo commented Jan 23, 2025

Add a Fourier basis.

Uh oh!

billbrod left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjvenditto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BalzaniEdoardo commented Jun 25, 2025

Uh oh!

BalzaniEdoardo commented Jun 25, 2025

Uh oh!

Uh oh!