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

Added glm.py examples in docstrings #249

Merged
merged 23 commits into from
Oct 28, 2024
Merged

Conversation

pranmod01
Copy link
Collaborator

@pranmod01 pranmod01 commented Oct 13, 2024

This PR adds examples for the glm.py public-facing commands. I added examples of how the function might be used with different parameters to get different results.

To be discussed: These are my first set of examples for functions. Because I might not thoroughly understand how these examples might be used in the context of neural data, I'm unsure if I should add more details and scenarios of how the functions can be used.

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2024

Codecov Report

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

Project coverage is 97.23%. Comparing base (6b3a1e8) to head (12a3b16).
Report is 28 commits behind head on development.

Files with missing lines Patch % Lines
src/nemos/glm.py 88.88% 2 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #249      +/-   ##
===============================================
+ Coverage        97.09%   97.23%   +0.14%     
===============================================
  Files               20       21       +1     
  Lines             1857     1884      +27     
===============================================
+ Hits              1803     1832      +29     
+ Misses              54       52       -2     

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

@billbrod
Copy link
Member

Pranati was having an issue where running isort locally was giving inconsistent results with the CI and with my local machine. Turned out the issue was PyCQA/isort#1790: there was a local folder named jax in her nemos directory, which made isort consider jax as an internal package, and changing how it was sorted. jax was a git submodule, and we're unsure how it ended up there. Deleting the directory caused isort to behave as expected.

@BalzaniEdoardo BalzaniEdoardo changed the base branch from main to development October 21, 2024 18:58
Copy link
Collaborator

@BalzaniEdoardo BalzaniEdoardo left a comment

Choose a reason for hiding this comment

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

This is almost good to go but we should have an example in the fit method of PopulationGLM, since it overrides the GLM one.

src/nemos/glm.py Outdated Show resolved Hide resolved
src/nemos/basis.py Outdated Show resolved Hide resolved
@pranmod01
Copy link
Collaborator Author

Updates made:

  1. Added example to the fit method of PopulationGLM
  2. Changed some wording based on Edoardo's comments
  3. Removed the basis example that overlapped with this branch to keep this branch clean of only GLM examples
  4. Regular formatting tox fixes

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 good! Couple more user-facing functions need examples, though @BalzaniEdoardo maybe some of these can be made private?

  • cast_to_jax
  • initialize_state

General notes:

  • While it's fine to have the imports in the examples right above where they get used, I think it's more standard to have all imports at the top (that's how scripts and notebooks are generally written) -- any particular reason you didn't do it that way?

src/nemos/basis.py Outdated Show resolved Hide resolved
src/nemos/glm.py Outdated Show resolved Hide resolved
src/nemos/glm.py Outdated Show resolved Hide resolved
src/nemos/glm.py Show resolved Hide resolved
src/nemos/glm.py Outdated Show resolved Hide resolved
src/nemos/glm.py Show resolved Hide resolved
@pranmod01
Copy link
Collaborator Author

For the comment about ordering the import statements -- I didn't put them all at the top and instead put them in the order that they are called just because I thought it would be easier for someone who didn't really understand the imports to see exactly how they are being used. But because the examples aren't too lengthy or technical, I can change the imports to the regular standard. Let me know!

Added an example to initialize_state but not cast_to_jax because the latter seems to be only used internally. Other than that, everything else is fixed from my end.

Copy link
Collaborator

@BalzaniEdoardo BalzaniEdoardo left a comment

Choose a reason for hiding this comment

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

Last very small round of comments, and then this is good to be merged. Thanks Pranati, very good job on this

src/nemos/glm.py Outdated Show resolved Hide resolved
src/nemos/glm.py Show resolved Hide resolved
src/nemos/glm.py Outdated Show resolved Hide resolved
src/nemos/glm.py Outdated Show resolved Hide resolved
src/nemos/glm.py Outdated Show resolved Hide resolved
src/nemos/glm.py Outdated Show resolved Hide resolved
src/nemos/glm.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 still think the definition of size for Xnew should be changed, but otherwise this looks good to me!

src/nemos/glm.py Outdated Show resolved Hide resolved
src/nemos/glm.py Show resolved Hide resolved
src/nemos/glm.py Outdated Show resolved Hide resolved
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
@BalzaniEdoardo
Copy link
Collaborator

This looks good! Couple more user-facing functions need examples, though @BalzaniEdoardo maybe some of these can be made private?

  • cast_to_jax
  • initialize_state

General notes:

  • While it's fine to have the imports in the examples right above where they get used, I think it's more standard to have all imports at the top (that's how scripts and notebooks are generally written) -- any particular reason you didn't do it that way?

Let's make 'cast_to_jax' private, keep 'initialize_state' public since batching as we have it now requires the user to call the method, if you check the relative how to guide in the docs.

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.

All looks good to me, thanks for your work on this Pranati!

@BalzaniEdoardo should we open an issue about making cast_to_jax private? (doing that probably doesn't belong here)

@BalzaniEdoardo BalzaniEdoardo self-requested a review October 28, 2024 17:55
@BalzaniEdoardo BalzaniEdoardo merged commit 0ac4b81 into development Oct 28, 2024
13 checks passed
@BalzaniEdoardo
Copy link
Collaborator

Thanks @pranmod01!

@BalzaniEdoardo BalzaniEdoardo deleted the glm_examples_section branch October 28, 2024 17:55
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