Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fourier #49

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Add fourier #49

wants to merge 15 commits into from

Conversation

BalzaniEdoardo
Copy link
Collaborator

  • Added FourierBasis to the basis module
  • tests and docs modified accordingly

@BalzaniEdoardo BalzaniEdoardo marked this pull request as draft October 14, 2023 13:33
@BalzaniEdoardo BalzaniEdoardo marked this pull request as ready for review December 19, 2023 17:02
Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should change the parametrization of this new basis, as discussed in my comments, and probably work a bit on how it's explained. I don't have any comments on how it's implemented, looks great.

# ### The Fourier Basis
# Another type of basis available is the Fourier Basis. Fourier basis are ideal to capture periodic and
# quasi-periodic patterns. Such oscillatory, rhythmic behavior is a common signature of many neural signals.
# Additionally, the Fourier basis has the advantage of being orthogonal, which simplifies the estimation and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably explain orthogonal here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still think this. at least a foot note or link

docs/examples/plot_1D_basis_function.py Outdated Show resolved Hide resolved
docs/examples/plot_1D_basis_function.py Outdated Show resolved Hide resolved
docs/examples/plot_1D_basis_function.py Outdated Show resolved Hide resolved
docs/examples/plot_1D_basis_function.py Outdated Show resolved Hide resolved
src/nemos/basis.py Outdated Show resolved Hide resolved
src/nemos/basis.py Outdated Show resolved Hide resolved
src/nemos/basis.py Outdated Show resolved Hide resolved
src/nemos/basis.py Outdated Show resolved Hide resolved
tests/test_basis.py Outdated Show resolved Hide resolved
"""
if self.n_basis_funcs < 1:
raise ValueError(
f"Object class {self.__class__.__name__} requires >= 1 basis elements. "
f"Object class {self.__class__.__name__} requires >= 0 basis elements. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the error message now matches the check, but shouldn't this actually at least 1 basis element? With max_freq=0 (just the DC term), you get a n_basis_funcs=1. And it doesn't make sense to have max_freq<0

# ### The Fourier Basis
# Another type of basis available is the Fourier Basis. Fourier basis are ideal to capture periodic and
# quasi-periodic patterns. Such oscillatory, rhythmic behavior is a common signature of many neural signals.
# Additionally, the Fourier basis has the advantage of being orthogonal, which simplifies the estimation and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still think this. at least a foot note or link

Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this parametrization much more. Still have some comments about explanation from the last round that need addressing, and then some small wording suggestions here.

Comment on lines +137 to +138
# Note that we are inverting the time axis of the basis because we are aiming
# for a cross-correlation, while np.convolve compute a convolution which would flip the time axis.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still think this

Comment on lines 150 to 152
# compute the phase and amplitude from the convolution
xcorr_phase = np.arctan2(np.hstack([[0], xcorr[n_freqs:]]), xcorr[:n_freqs])
xcorr_aplitude = np.sqrt(xcorr[:n_freqs] ** 2 + np.hstack([[0], xcorr[n_freqs:]]) ** 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still think this

docs/examples/plot_1D_basis_function.py Outdated Show resolved Hide resolved
docs/examples/plot_1D_basis_function.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6f210b4) 95.02% compared to head (a62712e) 95.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
+ Coverage   95.02%   95.34%   +0.32%     
==========================================
  Files          10       10              
  Lines         864      881      +17     
==========================================
+ Hits          821      840      +19     
+ Misses         43       41       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@kushalkolar kushalkolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments :)

Comment on lines +1241 to +1250
>>> import nemos as nmo
>>> import numpy as np
>>> n_samples, max_freq = 1000, 10
>>> basis = nmo.basis.FourierBasis(max_freq)
>>> eval_basis = basis.evaluate(np.linspace(0, 1, n_samples))
>>> sinusoid = np.cos(3 * np.arange(0, 1000) * np.pi * 2 / 1000.)
>>> conv = [np.convolve(eval_basis[::-1, k], sinusoid, mode='valid')[0] for k in range(2*max_freq+1)]
>>> fft = np.fft.fft(sinusoid)
>>> print('FFT power: ', np.round(np.real(fft[:max_freq]), 4))
>>> print('Convolution: ', np.round(conv[:max_freq], 4))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way in mkdocs to set this as a python codeblock? Can you remove the >>>? Because right now in the docs the >>> shows up and when you copy it copies with the >>> so a user can't just copy and paste and run from the docs 😄

for example at the bottom here the arrows render and get copied: https://nemos.readthedocs.io/en/latest/reference/nemos/utils/#nemos.utils.pytree_map_and_reduce

(sample_pts,) = self._check_evaluate_input(sample_pts)
# assumes equi-spaced samples.
if sample_pts.shape[0] / np.max(self._frequencies) < 2:
raise ValueError("Not enough samples, aliasing likely to occur!")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe report sample_pts.shape[0] and max(self._frequencies) to the user?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants