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

Reparametrize raisedcosine #50

Merged
merged 36 commits into from
Dec 19, 2023
Merged

Reparametrize raisedcosine #50

merged 36 commits into from
Dec 19, 2023

Conversation

BalzaniEdoardo
Copy link
Collaborator

@BalzaniEdoardo BalzaniEdoardo commented Oct 14, 2023

This edits introduce a one extra hyper-parameters for initializing the linear raised cosine bumps, and an two one for the log-raised cosines:

  • width: this hyper-parameter is set for both linear- and log-spaced raised cosines. It controls the width of the cosine bumps, default is 2. The original implmentation was assuming this to be 1, but that produces aliasing
  • time_scaling: this hyper-parameter is available only for log-spaced raised cosines and controls the logarithmic stretch magnitude. as time_scaling approaches to 0, the transformation becomes linear. large time_scaling result in basis that are tightly concentrated around t=0
  • enforce_decay_to_zero: boolean, if true, it enforce the last basis to decay to zero.

@BalzaniEdoardo BalzaniEdoardo marked this pull request as draft October 14, 2023 13:44
@BalzaniEdoardo BalzaniEdoardo marked this pull request as ready for review December 15, 2023 17:21
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2dcf1b2) 94.88% compared to head (664169d) 95.04%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   94.88%   95.04%   +0.16%     
==========================================
  Files          10       10              
  Lines         840      868      +28     
==========================================
+ Hits          797      825      +28     
  Misses         43       43              

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

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.

This looks pretty much good to me! Some small changes scattered throughout, and then the following things I think we should discuss:

  • Basis docstrings. I think they should have a line or two before parameters that describe what the basis looks like and when you would use them. E.g., do they tile uniformly, are they each about the same size, what types of variables make sense (visual input, head direction, etc.). And then the footnote to references belongs in those lines (it looks weird in the docstring in jupyter or whatever to have the link right at the top). This is probably out-of-scope for this PR, in which case we should open an issue for it.
  • We need an explanation of the particularities of the different parameters for the Raised Cosine parameters. Much of that will end up in our separate repo, but we need some here. Do we want to hold off on that until we've worked through it in the separate repo and then add something here? (in which case, we should open an issue)

.github/workflows/ci.yml 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 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
BalzaniEdoardo and others added 4 commits December 19, 2023 12:46
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
@BalzaniEdoardo
Copy link
Collaborator Author

I changed the code so that the peak location is computed so that the decay to zero of the last basis element happens as late as possible. this is not equivalent to what we had before, (it would be if i use np.ceil(width) instead of width to compute the last peak location.

Comment on lines +983 to +984
# - as the time_scaling tends to inf, basis will be small and dense around 0 and
# progressively larger and less dense towards 1.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# - as the time_scaling tends to inf, basis will be small and dense around 0 and
# progressively larger and less dense towards 1.
# - as the time_scaling tends to inf, basis will be small and dense around 0 and
# progressively larger and less dense towards 1.

src/nemos/basis.py Outdated Show resolved Hide resolved
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 this looks good to me!

@BalzaniEdoardo BalzaniEdoardo merged commit 6f210b4 into main Dec 19, 2023
3 checks passed
@BalzaniEdoardo BalzaniEdoardo deleted the reparametrize_raisedcosine branch December 19, 2023 21:11
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.

3 participants