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

code review: GLM #1720

Open
josef-pkt opened this issue Jun 2, 2014 · 3 comments
Open

code review: GLM #1720

josef-pkt opened this issue Jun 2, 2014 · 3 comments

Comments

@josef-pkt
Copy link
Member

  • why do we calculate pinv_wexog in GLM.initialize? It's not used AFAICS.
    normalized_cov_params is calculated with it, but that's also unused.
  • fit "state" attached to the model: several fit results are attached to model (e.g. mu). It's part of the design for the iterative estimation. I don't see a problem, but it would be better to detach those from the model, once they are attached to the results instance.

naming

  • family predict versus fitted: not clear which is which. ?
    (difficult because statistics and econometrics differ, link versus inverse-link)
@josef-pkt
Copy link
Member Author

minor: copy offset only if we have to change it:

        if hasattr(self, 'offset'):
            #offset = self.offset.copy()
            offset = self.offset
        if hasattr(self, 'exposure'):
            #offset += self.exposure
            offset = offset + self.exposure

kshedden added a commit to kshedden/statsmodels that referenced this issue Jun 3, 2014
@kshedden kshedden mentioned this issue Jun 4, 2014
josef-pkt pushed a commit to josef-pkt/statsmodels that referenced this issue Jul 9, 2014
PierreBdR pushed a commit to PierreBdR/statsmodels that referenced this issue Sep 2, 2014
@josef-pkt josef-pkt added this to the 0.10 milestone Oct 11, 2017
@josef-pkt
Copy link
Member Author

bump for 0.10

I just saw again the first why? I don't see any reason.

in GLM.initialize

        self.pinv_wexog = np.linalg.pinv(self.exog)
        self.normalized_cov_params = np.dot(self.pinv_wexog,
                                            np.transpose(self.pinv_wexog))

And the GLMResults.__init__ attaches pinv_wexog for no reason that I can see

my guess is that these are leftovers from earlier versions of fit.

@josef-pkt
Copy link
Member Author

pinv in initialize has been removed in #4624

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
0.10
Awaiting triage
Development

No branches or pull requests

1 participant