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

[WIP] distribution classes #390

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

pavanramkumar
Copy link
Collaborator

@pavanramkumar pavanramkumar commented Aug 16, 2020

closes #350

TODOs

  • make it work for all distribution classes
  • add tests for BaseDistribution class
  • make tests and examples pass locally
  • remove unused hidden methods from pyglmnet.py (currently commented out)
  • migrate simulate_glm() functionality into simulate() methods of respective classes
  • migrate metrics definitions that depend on log_likelihood into distribution classes (pseudo_R2, deviance)
  • add an example to show how users should roll out their own distribution class (e.g., using jax numpy)
  • ensure Probit and Binomial work correctly
  • check if all docstrings still make sense
  • update API reference
  • update what's new

@geektoni
Copy link
Contributor

💃 💃

Comment on lines 299 to 301
def __init__(self):
"""init."""
pass
Copy link
Member

Choose a reason for hiding this comment

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

you probably don't need to redefine it since you're inheriting from BaseDistribution ?

@jasmainak
Copy link
Member

exciting stuff! I think it significantly improves our repository :-D

I would love to see an example with autograd or something like that

if isinstance(distr, BaseDistribution):
return distr

elif distr == 'gaussian':
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can change this huge if-else to something in which we dynamically instantiate the correct class given the string. I was thinking of something like this https://softwareengineering.stackexchange.com/a/351394, in which we have a dictionary which is holding all the required classes.

from pyglmnet.distributions import Binomial


class CustomBinomial(Binomial):
Copy link
Member

Choose a reason for hiding this comment

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

@pavanramkumar I'm not sure this is legit. The log_likelihood function for Binomial does not call self.mu or does it? Is the log likelihood the log likelihood taking into account the link function?

@geektoni
Copy link
Contributor

Quick question. If I wanted to contribute on this one here, should I fork your repo @pavanramkumar or do you know if there is any way to push directly to this PR?

@jasmainak
Copy link
Member

I think forking might be the easiest. But maybe @pavanramkumar should confirm first if he has any local changes that have not been pushed?

@pavanramkumar
Copy link
Collaborator Author

Thanks for picking this up @geektoni ! Please go ahead and fork.

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.

Pass distribution objects into GLM and GLMCV class
3 participants