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

Identity basis #282

Merged
merged 79 commits into from
Dec 20, 2024
Merged

Identity basis #282

merged 79 commits into from
Dec 20, 2024

Conversation

BalzaniEdoardo
Copy link
Collaborator

In this PR I add an identity basis function and a basis function that capture raw history.

I added a few how-to guide that showcase the use of these basis for:

  1. Including custom predictors in a composite basis.
  2. Create a design from categorical variables.
  3. Fit a coupled GLM.

@BalzaniEdoardo BalzaniEdoardo marked this pull request as ready for review December 20, 2024 14:52
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2024

Codecov Report

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

Project coverage is 97.27%. Comparing base (a510ef3) to head (89f279b).
Report is 60 commits behind head on development.

Files with missing lines Patch % Lines
src/nemos/basis/_identity.py 98.03% 1 Missing ⚠️
src/nemos/basis/_transformer_basis.py 97.61% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #282      +/-   ##
===============================================
+ Coverage        96.13%   97.27%   +1.13%     
===============================================
  Files               34       35       +1     
  Lines             2642     2750     +108     
===============================================
+ Hits              2540     2675     +135     
+ Misses             102       75      -27     

☔ 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.

It looks like this PR is showing the changes from #280 in the diff, do you know why?

Otherwise, these looks generally good, though I think this means we're going to have to go through the existing how-tos ,etc to see where to use the new bases (at least in the head direction tutorial). Can we make an issue for that?

The new how-tos all looks like they match what they are in diataxis, i.e., very short and answering a single question. I think we'll need to go back through the existing ones and update / move them then, and I think we should consider changing the layout of the landing page for them: make the cards smaller, only show the top-level title, which should be very descriptive. This should also be a separate issue.

src/nemos/basis/_identity.py Outdated Show resolved Hide resolved
src/nemos/basis/_identity.py Show resolved Hide resolved
src/nemos/basis/_identity.py Outdated Show resolved Hide resolved
src/nemos/basis/basis.py Outdated Show resolved Hide resolved
docs/background/basis/README.md Show resolved Hide resolved
docs/how_to_guide/custom_predictors.md Show resolved Hide resolved
docs/how_to_guide/custom_predictors.md Show resolved Hide resolved
docs/how_to_guide/raw_history_feature.md Outdated Show resolved Hide resolved
docs/how_to_guide/raw_history_feature.md Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

why not do this one using the data from https://nemos--282.org.readthedocs.build/en/282/tutorials/plot_02_head_direction.html? then we can visualize the outputs (and point them to the full tutorial for more details)

Copy link
Member

Choose a reason for hiding this comment

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

regardless, I think we should visualize the model coefficients we get

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think it requires too much pre-processing for this session. I'll print the coefficients

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also, i would need more explaining about the data, maybe filter a subset of unit and samples... I think it is better to go straight to the point. If we had a sample data which has already a sub-population and a short duration we could load that (like the iris dataset for sklearn)

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I think this is fine, but it would be nice to think about whether there is a meaningful dataset we could use. because it's nice to plot the coefficients and have them look meaningful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you are right

Comment on lines 113 to 115
You can directly pass a [`pandas.DataFrame`](https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.html) to the
`fit` method, as long as it can be internally converted to a floating-point array. The conversion is handled automatically,
so no additional steps are required on your part.
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
You can directly pass a [`pandas.DataFrame`](https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.html) to the
`fit` method, as long as it can be internally converted to a floating-point array. The conversion is handled automatically,
so no additional steps are required on your part.
You can directly pass a [`pandas.DataFrame`](https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.html) to the
`fit` method, as long as it can be internally converted to a floating-point array (that means, for example, that you can't have a column whose values are strings). The conversion is handled automatically, by calling `design_df.values`,
so no additional steps are required on your part.

bas = nmo.basis.IdentityEval() + nmo.basis.BSplineEval(5)

# Compute features
X = bas.compute_features(design_df, speed)
Copy link
Member

Choose a reason for hiding this comment

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

but it will fail if they try to do use a dataframe with non-numeric values anyway, right?

@BalzaniEdoardo
Copy link
Collaborator Author

?

It looks like this PR is showing the changes from #280 in the diff, do you know why?
No idea, I might have branched from set_shape_basis_method and this confused the history. I am not sure it makes sense

Otherwise, these looks generally good, though I think this means we're going to have to go through the existing how-tos ,etc to see where to use them. Can we make an issue for that?
done

The new how-tos all looks like they match what they are in diataxis, i.e., very short and answering a single question. I think we'll need to go back through the existing ones and update / move them then, and I think we should consider changing the layout of the landing page for them: make the cards smaller, only show the top-level title, which should be very descriptive. This should also be a separate issue.

I agree with both points

@BalzaniEdoardo BalzaniEdoardo merged commit 3dc7e29 into development Dec 20, 2024
13 checks passed
@BalzaniEdoardo BalzaniEdoardo deleted the identity_basis branch December 20, 2024 20:01
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