Skip to content

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