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

Poisson Model #140

Merged
merged 14 commits into from
Sep 21, 2022
Merged

Poisson Model #140

merged 14 commits into from
Sep 21, 2022

Conversation

ilan-gold
Copy link
Collaborator

It appears that for the test that is failing, the threshold is .2 for the mean and the real average deviation is 0.27 - I've heard previously that these tests can be unreliable though so not sure how to proceed. A few other things:

  1. I largely copied the closed initializers (since I still don't fully understand what they are doing) and removed the scale aspect of them
  2. I am not sure how the bounds method is derived so I copied that as well - is there something in the manuscript?
  3. Similar to the normal case, we have arguments we don't need - here though, we don't need anything related to scale. I think given that 2/3 of the models that we use now deviate from the standard implementation, maybe all of this should be in kwargs? Not sure what the idiomatic python thing is to do.
  4. If the failing test is indeed wrong, what are some good ways to debug here?

@ilan-gold ilan-gold marked this pull request as draft March 27, 2022 18:16
@ilan-gold ilan-gold changed the title Poisson Poisson Model Apr 11, 2022
@ilan-gold
Copy link
Collaborator Author

Tests fixed!

@ilan-gold ilan-gold requested a review from picciama April 11, 2022 15:10
@ilan-gold ilan-gold marked this pull request as ready for review April 11, 2022 15:10
Copy link
Collaborator

@picciama picciama left a comment

Choose a reason for hiding this comment

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

Overall I'd say this is very much done. Only some minor things I've spotted.
Most importantly, I'd say init_theta_scale should really be removed and maybe we can reuse some functions by putting them into the base_glm submodule. But that might be better to do once all the noise models are implemented and merged.

batchglm/models/glm_poisson/model.py Show resolved Hide resolved
batchglm/models/glm_poisson/model.py Show resolved Hide resolved
batchglm/models/glm_poisson/model.py Show resolved Hide resolved
batchglm/models/glm_poisson/utils.py Show resolved Hide resolved
batchglm/models/glm_poisson/utils.py Outdated Show resolved Hide resolved
batchglm/train/numpy/glm_poisson/estimator.py Outdated Show resolved Hide resolved
batchglm/models/glm_poisson/utils.py Show resolved Hide resolved
batchglm/train/numpy/glm_poisson/estimator.py Outdated Show resolved Hide resolved
batchglm/train/numpy/glm_poisson/estimator.py Outdated Show resolved Hide resolved
batchglm/train/numpy/glm_poisson/model_container.py Outdated Show resolved Hide resolved
@ilan-gold ilan-gold merged commit 8022560 into development Sep 21, 2022
@ilan-gold ilan-gold deleted the ig/poisson branch September 21, 2022 13:44
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.

2 participants