Initialization

[BalzaniEdoardo]

In this PR I fix the initialization issue in #243;

Additionally, I removed the parallel testing from the default configuration of pytest, but added the parallelization it in the tox.ini call instead.

The reason for this is that debugging tests is much easier without parallelization; Additionally, when debugging, one often needs to run a single test function or a few of them, and the overhead of starting workers makes it slower the single core option.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.23%. Comparing base (32132b1) to head (66ab701).
Report is 520 commits behind head on development.

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #252      +/-   ##
===============================================
- Coverage        97.30%   97.23%   -0.07%     
===============================================
  Files               18       21       +3     
  Lines             1669     1884     +215     
===============================================
+ Hits              1624     1832     +208     
- Misses              45       52       +7     

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

[billbrod]

Some notes:

• Probably worth copying over your comment in the linked issue that describes what is being done here (and why).
• Also, we should probably get in the habit of adding Examples sections in the docstrings of user-facing functions as we write the function.
• There look like there are a lot of changes to the test_glm.py file that are just black being run on it for the first time. Should we include running black on the test files?

[billbrod]

there are other contexts where this fails than having a nan in the output right? we should test them instead, yeah?

[BalzaniEdoardo]

hard to hit exception due to fitting not working. we can try using a function that cannot be inverted, that may lead to the search to diverge to inf?

[BalzaniEdoardo]

Added an additional test in which the link function cannot be inverted to get the mean rate

[billbrod]

In general, we can't provide any info as to why this failed, right? I think this message is a bit opaque for users (for example), but maybe we are treating users who use a non-standard link function as advanced.

I do think we could have initialize_intercept_matching_mean_rate catch this ValueError and raise a more specific error, saying that we were unable to set the initial parameters to match the mean firing rate.

[BalzaniEdoardo]

Copying over my comments from #243.:

Comment 1

Hi Luigi, first of all, thanks for this issue, really appreciate all the details provided. I am looking into it now; The initialization was meant to find a numerical inverse of the link function.

For exp, we know it is log, but since we allow to pass any arbitrary non-linearity I wanted a more general approach; I had that issue already, and I thought I'd fixed it using scipy root finding to get the inverse, which was stable enough on my tests.
I'll try your synthetic data on a more recent branch, like development to see if the issue is already resolved and get back to you soon.

Comment 2

@vigji I probably won't be able to merge this in by this week because we are under a deadline, but I created a branch that fixes the issue by:

• Using known inverse link function when possible (log, and inverse soft-plus).
• If not possible, it will try to invert the link function numerically. In the new implementation we use a loop over the neurons and call scipy.optimize.root_scalar since for each neuron can be treated independently. This makes the problem much simpler, and more stable. I tried it out on the synthetic data you generated and works well.

If you want to test it out, try this branch. If you want to check the numerical inversion on the real data too, pass as a link function lambda x: jax.numpy.exp(x) instead of the exponential directly.