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

BUG: Allow all appropriate links in a Family #7816

Merged
merged 1 commit into from Oct 24, 2021
Merged

BUG: Allow all appropriate links in a Family #7816

merged 1 commit into from Oct 24, 2021

Conversation

tncowart
Copy link
Contributor

The list of links in a Family (Poisson, Gaussian, etc.) included the
"aliased" link type instead of the primary link type, when possible.
Poisson, for example, include link.log instead of link.Log.

Using the aliased class in the family link list results in suprising
behavior like this:

In [1]: import statsmodels.genmod.families as F

In [2]: import statsmodels.genmod.families.links as L

In [3]: F.Poisson(link=L.log())
Out[3]: <statsmodels.genmod.families.family.Poisson at 0x111da15b0>

In [4]: F.Poisson(link=L.Log())
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-4-47050bb1e621> in <module>
----> 1 F.Poisson(link=L.Log())

~/Documents/old/statsmodels/statsmodels/genmod/families/family.py in __init__(self, link)
    399         if link is None:
    400             link = L.log()
--> 401         super(Poisson, self).__init__(link=link, variance=Poisson.variance)
    402
    403     def _resid_dev(self, endog, mu):

~/Documents/old/statsmodels/statsmodels/genmod/families/family.py in __init__(self, link, variance)
     83             raise TypeError(warnmssg)
     84         else:
---> 85             self.link = link
     86         self.variance = variance
     87

~/Documents/old/statsmodels/statsmodels/genmod/families/family.py in _setlink(self, link)
     64             if not validlink:
     65                 errmsg = "Invalid link for family, should be in %s. (got %s)"
---> 66                 raise ValueError(errmsg % (repr(self.links), link))
     67
     68     def _getlink(self):

ValueError: Invalid link for family, should be in [<class 'statsmodels.genmod.families.links.log'>, <class 'statsmodels.genmod.families.links.identity'>, <class 'statsmodels.genmod.families.links.sqrt'>]. (got <statsmodels.genmod.families.links.Log object at 0x111e7efa0>)

L.log works but L.Log doesn't, even though L.log is an simply an "alias"
of L.Log.

By changing the family link lists to use "unaliased" classes (the parent
classes of the various aliases) both L.log and L.Log work as expected.

  • closes #xxxx
  • tests added / passed.
  • code/documentation is well formatted.
  • properly formatted commit message. See
    NumPy's guide.

Notes:

  • It is essential that you add a test when making code changes. Tests are not
    needed for doc changes.
  • When adding a new function, test values should usually be verified in another package (e.g., R/SAS/Stata).
  • When fixing a bug, you must add a test that would produce the bug in main and
    then show that it is fixed with the new code.
  • New code additions must be well formatted. Changes should pass flake8. If on Linux or OSX, you can
    verify you changes are well formatted by running
    git diff upstream/main -u -- "*.py" | flake8 --diff --isolated
    
    assuming flake8 is installed. This command is also available on Windows
    using the Windows System for Linux once flake8 is installed in the
    local Linux environment. While passing this test is not required, it is good practice and it help
    improve code quality in statsmodels.
  • Docstring additions must render correctly, including escapes and LaTeX.

@pep8speaks
Copy link

pep8speaks commented Oct 21, 2021

Hello @tncowart! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-10-22 01:16:37 UTC

@josef-pkt
Copy link
Member

unrelated random failure, missing seed

(F.InverseGaussian, inverse_gaussian_links),
(F.NegativeBinomial, negative_bionomial_links),
(F.Tweedie, tweedie_links)
])
Copy link
Member

Choose a reason for hiding this comment

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

define the list outside, then it doesn't need to be repeated

cases = [....

@josef-pkt
Copy link
Member

Thanks, I think this looks good.
I'm running into this also every once in a while.

closes #1509

The list of links in a Family (Poisson, Gaussian, etc.) included the
"aliased" link type instead of the primary link type, when possible.
Poisson, for example, include link.log instead of link.Log.

Using the aliased class in the family link list results in suprising
behavior like this:

```
In [1]: import statsmodels.genmod.families as F

In [2]: import statsmodels.genmod.families.links as L

In [3]: F.Poisson(link=L.log())
Out[3]: <statsmodels.genmod.families.family.Poisson at 0x111da15b0>

In [4]: F.Poisson(link=L.Log())
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-4-47050bb1e621> in <module>
----> 1 F.Poisson(link=L.Log())

~/Documents/old/statsmodels/statsmodels/genmod/families/family.py in __init__(self, link)
    399         if link is None:
    400             link = L.log()
--> 401         super(Poisson, self).__init__(link=link, variance=Poisson.variance)
    402
    403     def _resid_dev(self, endog, mu):

~/Documents/old/statsmodels/statsmodels/genmod/families/family.py in __init__(self, link, variance)
     83             raise TypeError(warnmssg)
     84         else:
---> 85             self.link = link
     86         self.variance = variance
     87

~/Documents/old/statsmodels/statsmodels/genmod/families/family.py in _setlink(self, link)
     64             if not validlink:
     65                 errmsg = "Invalid link for family, should be in %s. (got %s)"
---> 66                 raise ValueError(errmsg % (repr(self.links), link))
     67
     68     def _getlink(self):

ValueError: Invalid link for family, should be in [<class 'statsmodels.genmod.families.links.log'>, <class 'statsmodels.genmod.families.links.identity'>, <class 'statsmodels.genmod.families.links.sqrt'>]. (got <statsmodels.genmod.families.links.Log object at 0x111e7efa0>)
```

L.log works but L.Log doesn't, even though L.log is an simply an "alias"
of L.Log.

By changing the family link lists to use "unaliased" classes (the parent
classes of the various aliases) both L.log and L.Log work as expected.
@josef-pkt
Copy link
Member

all green and LGTM, merging

@tncowart Thank you for the PR

@josef-pkt josef-pkt merged commit d990e32 into statsmodels:main Oct 24, 2021
bashtage pushed a commit to bashtage/statsmodels that referenced this pull request Nov 5, 2021
BUG: Allow all appropriate links in a Family
bashtage added a commit that referenced this pull request Nov 5, 2021
BACKPORT: Merge pull request #7816 from tncowart/unalias_links
@bashtage bashtage removed the backport label Nov 5, 2021
@josef-pkt josef-pkt added this to the 0.13.1 milestone Feb 21, 2023
@bashtage bashtage modified the milestones: 0.13.1, 0.14 Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants