Provide a range for basis #191

BalzaniEdoardo · 2024-07-16T22:48:49Z

Setting basis range

This PR addresses #178, allowing to set a fixed range for the basis.

This is a useful when,

We want to discard outliers, for example position tracking artifacts or similar
We want to fit the same model with the same basis configuration over different runs of an experiment.

The vmin/vmax are set at initialization of the basis, so that multiple calls to Basis.compute_features will return same result every time and do not require extra arguments.

vmin and vmax are get only properties.

Behavior of (`call` and `compute_features`):

The default behavior is unchanged. If vmin/vmax are not provided, they are set to the nanmin/nanmax respectively when the samples are provided.
If vmin/vmax are provided, the range starts/ends at vmin/vmax. Any sample out of range will return NaN.

Behavior of`evaluate_on_grid` (differs since it does not receive any sample):

if both vmin/vmax are provided, evaluates on an equispaced grid between vmin and vmax.
If only vmin is provided, evaluates on an equispaced grid between vmin and vmin + 1.
If only vmax is provided, evaluates on an equispaced grid between vmax - 1 and vmax.
If no value is provided, evaluates on an equispaced grid between 0 and 1.

from nemos.basis import BSplineBasis
import numpy as np


x = np.arange(10)
basis = BSplineBasis(5, vmin=1, vmax=8)

# this uses the values passed at initializaiton vmin=1, vmax=8
y = basis.compute_features(x)
y = basis(x)

BalzaniEdoardo · 2024-07-16T22:49:20Z

TODO

Add documentation in background

codecov-commenter · 2024-07-16T22:56:19Z

Codecov Report

Attention: Patch coverage is 96.92308% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.10%. Comparing base (fceea56) to head (e3d4a32).
Report is 90 commits behind head on development.

Files	Patch %	Lines
src/nemos/basis.py	96.55%	2 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #191      +/-   ##
===============================================
- Coverage        97.23%   97.10%   -0.13%     
===============================================
  Files               15       18       +3     
  Lines             1484     1626     +142     
===============================================
+ Hits              1443     1579     +136     
- Misses              41       47       +6

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

src/nemos/basis.py

billbrod

Basic ideas here all look good to me, but I've got some higher level questions:

Not sure how I feel about setting just one of vmin/vmax. Picking vmin+1 / vmax-1 feels very arbitrary. why not require both? we could do that by changing the argument to vrange and requiring it to be a 2-tuple
this PR pulls the eval and conv modes of the basis further apart. do we want to consider making them separate classes? or something else, but it feels weird to have fully half of the arguments be ignored in one mode and the other half in the other mode.
not necessarily part of this PR, but: I don't know how to pull up the docstring of __call__. what comes up is the docstring of the class. that seems ... like a problem. if I'm missing something, let me know, otherwise I can create an issue
for the raised cosine basis objects, they only give reasonable values on the interval [0,1]. So vmin/vmax should lie within that interval as well.
for those objects, it's weird that the log never rescales and the linear can optionally do so. shouldn't they both always rescale? did we discuss this at some point? the other basis objects never rescale, right? but it feels like rescaling means something different

docs/background/plot_01_1D_basis_function.py

src/nemos/validation.py

src/nemos/basis.py

billbrod · 2024-07-19T18:55:59Z

src/nemos/basis.py

+                sample_pts, knot_locs, order=self.order, der=0, outer_ok=False
+            )
+        else:
+            basis_eval = np.full((sample_pts.shape[0], self.n_basis_funcs), np.nan)


I'm confused as how this can be hit, if we raise a ValueError if all sample points lie outside vmin/vmax

(note this shows up in multiple basis objects)

yep, It wasn't the case at the beginning but then I changed my mind and this is a leftover

billbrod · 2024-07-19T18:57:39Z

src/nemos/basis.py

-        -------
-        basis_funcs :
-            Raised cosine basis functions, shape (n_samples, n_basis_funcs).
+        rescale_samples :


why is this only an option for this basis?

I realize this isn't new, but it feels odd, given the new behavior

also the docstring says that it rescales to 0,1, but now it depends on vmin/vmax

removed, and added a private flag at init that define if the RaisedCosineLinear should rescale or not (if called by the subclass it should not, otherwise it always have to)

Co-authored-by: William F. Broderick <billbrod@gmail.com>

BalzaniEdoardo · 2024-07-19T21:58:29Z

Basic ideas here all look good to me, but I've got some higher level questions:

* Not sure how I feel about setting just one of vmin/vmax. Picking vmin+1 / vmax-1 feels very arbitrary. why not require both? we could do that by changing the argument to `vrange` and requiring it to be a 2-tuple

This has been addressed (bounds is the name of the variable)

* this PR pulls the eval and conv modes of the basis further apart. do we want to consider making them separate classes? or something else, but it feels weird to have fully half of the arguments be ignored in one mode and the other half in the other mode.

We should discuss this with Sofia and @gviejo. But definitely not something we should tackle in this PR.

* not necessarily part of this PR, but: I don't know how to pull up the docstring of `__call__`. what comes up is the docstring of the class. that seems ... like a problem. if I'm missing something, let me know, otherwise I can create an issue

Interesting...

* for the raised cosine basis objects, they only give reasonable values on the interval [0,1]. So vmin/vmax should lie within that interval as well.

This has been resolved. RaisedCosineLinear should always be called with the rescale_sample flag set to true. The Log instead must call the super class call with the rescale_sample set to False.

* for those objects, it's weird that the log never rescales and the linear can optionally do so. shouldn't they both always rescale? did we discuss this at some point? the other basis objects never rescale, right? but it feels like rescaling means something different

They both have a single possible behavior, I was exposing the parameter by mistake.

billbrod

I have a couple points dropped throughout that I think need addressing, basically juts by adding a comment, but then I think this is good to go. I think using a single bounds argument is much cleaner.

src/nemos/basis.py

Co-authored-by: William F. Broderick <billbrod@gmail.com>

…to min_max_basis

Co-authored-by: William F. Broderick <billbrod@gmail.com>

…to min_max_basis

BalzaniEdoardo added 5 commits July 9, 2024 14:45

added vmin, vmax option for rescaling

2b374b9

added rescaling to all but exp decay basis

505a90b

added rescaling to all but exp decay basis

e65d75d

modified basis

008df44

fixed tests

9be4ad3

BalzaniEdoardo added 12 commits July 17, 2024 16:43

change _call logic

9f0dc51

fixed basis and tests

956ee6a

bugfix cosyne

fe9a65d

changes

fb84bf2

reverted changes

874b1f1

removed unused

54a5872

renamed tests

6d92592

fixed tests

ed3545a

added tests for basis

e83d207

linted

b3f6b2b

simplified call logic

992a1e1

removed docsrtings incorrect info

7ae9b52

BalzaniEdoardo commented Jul 18, 2024

View reviewed changes

src/nemos/basis.py Show resolved Hide resolved

BalzaniEdoardo marked this pull request as ready for review July 18, 2024 20:10

BalzaniEdoardo assigned gviejo and billbrod Jul 18, 2024

billbrod requested changes Jul 19, 2024

View reviewed changes

BalzaniEdoardo and others added 6 commits July 19, 2024 16:52

switched to bounds

897dca6

fixed tests basis

cca8075

linted

998cfd3

fixed docs

0c31798

improved docstrings

ad9ce69

Update src/nemos/basis.py

b39d23b

Co-authored-by: William F. Broderick <billbrod@gmail.com>

removed unused if else

e2a6a92

black

ea33517

BalzaniEdoardo requested a review from billbrod July 19, 2024 22:04

Merge branch 'development' into min_max_basis

87485a5

billbrod approved these changes Jul 26, 2024

View reviewed changes

billbrod mentioned this pull request Jul 26, 2024

Should we separate basis objects' eval and conv mode? #202

Open

BalzaniEdoardo and others added 12 commits July 27, 2024 12:20

Update src/nemos/basis.py

7c99155

Co-authored-by: William F. Broderick <billbrod@gmail.com>

Update src/nemos/basis.py

390e84a

Co-authored-by: William F. Broderick <billbrod@gmail.com>

moved to setter, surfaced exception

a3e1ebd

Update src/nemos/basis.py

631dcd8

Co-authored-by: William F. Broderick <billbrod@gmail.com>

Merge branch 'min_max_basis' of github.com:flatironinstitute/nemos in…

02abd27

…to min_max_basis

set to numnerical precision

2316f19

Update src/nemos/basis.py

3e12e39

Co-authored-by: William F. Broderick <billbrod@gmail.com>

added example for mspline

5ae3d06

Update src/nemos/basis.py

5735a3b

Co-authored-by: William F. Broderick <billbrod@gmail.com>

linted

3cfe3e0

Merge branch 'min_max_basis' of github.com:flatironinstitute/nemos in…

06db929

…to min_max_basis

merged

e3d4a32

BalzaniEdoardo merged commit 8784eac into development Jul 27, 2024
11 checks passed

BalzaniEdoardo deleted the min_max_basis branch July 27, 2024 17:09

BalzaniEdoardo mentioned this pull request Jul 27, 2024

pre-defined range/knots for basis #178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a range for basis #191

Provide a range for basis #191

BalzaniEdoardo commented Jul 16, 2024 •

edited

Loading

BalzaniEdoardo commented Jul 16, 2024 •

edited

Loading

codecov-commenter commented Jul 16, 2024 •

edited

Loading

billbrod left a comment

billbrod Jul 19, 2024

billbrod Jul 19, 2024

BalzaniEdoardo Jul 19, 2024

billbrod Jul 19, 2024

billbrod Jul 19, 2024

billbrod Jul 19, 2024

BalzaniEdoardo Jul 19, 2024

BalzaniEdoardo commented Jul 19, 2024

billbrod left a comment

Provide a range for basis #191

Provide a range for basis #191

Conversation

BalzaniEdoardo commented Jul 16, 2024 • edited Loading

Setting basis range

Behavior of (__call__ and compute_features):

Behavior ofevaluate_on_grid (differs since it does not receive any sample):

BalzaniEdoardo commented Jul 16, 2024 • edited Loading

codecov-commenter commented Jul 16, 2024 • edited Loading

Codecov Report

billbrod left a comment

Choose a reason for hiding this comment

billbrod Jul 19, 2024

Choose a reason for hiding this comment

billbrod Jul 19, 2024

Choose a reason for hiding this comment

BalzaniEdoardo Jul 19, 2024

Choose a reason for hiding this comment

billbrod Jul 19, 2024

Choose a reason for hiding this comment

billbrod Jul 19, 2024

Choose a reason for hiding this comment

billbrod Jul 19, 2024

Choose a reason for hiding this comment

BalzaniEdoardo Jul 19, 2024

Choose a reason for hiding this comment

BalzaniEdoardo commented Jul 19, 2024

billbrod left a comment

Choose a reason for hiding this comment

BalzaniEdoardo commented Jul 16, 2024 •

edited

Loading

Behavior of (`call` and `compute_features`):

Behavior of`evaluate_on_grid` (differs since it does not receive any sample):

BalzaniEdoardo commented Jul 16, 2024 •

edited

Loading

codecov-commenter commented Jul 16, 2024 •

edited

Loading