PERF: GLM - don't use pinv in initialize #4624
Conversation
thequackdaddy
changed the title
thequackdaddy
force-pushed
the
ignore_pinv
branch
from
bdbb0d3
to
5afdb82
Compare
thequackdaddy
changed the title
Codecov Report
@@ Coverage Diff @@
## master #4624 +/- ##
==========================================
- Coverage 80.28% 80.28% -0.01%
==========================================
Files 564 564
Lines 85664 85661 -3
Branches 9681 9681
==========================================
- Hits 68772 68769 -3
+ Misses 14664 14663 -1
- Partials 2228 2229 +1
Continue to review full report at Codecov.
|
|
I'm pretty sure this is correct (there is no use for pinv_wexog that is based on exog), and no test failures merging |
REF/PERF: GLM - remove unused pinv in initialize closes statsmodels#4622
| @@ -114,8 +114,6 @@ class GLM(base.LikelihoodModel): | |||
| See Parameters. | |||
| normalized_cov_params : array | |||
There was a problem hiding this comment.
should this also be removed from the docstring? the attribute is removed below
There was a problem hiding this comment.
If so, the docstring entry in GLMResults needs to be updated too.
There was a problem hiding this comment.
Yes that attribute is gone and should also be removed from the docstring.
normalized_cov_params in GLMResults, is the real one evalutated at the final estimate.
There was a problem hiding this comment.
OK. This is the kind of thing that I'd be happy to address in a quick-turnaround PR if e.g. @bashtage were anointed to review+merge.
There was a problem hiding this comment.
A clear docstring fix is usually a fast merge,
(as long as you don't try to sneak in any other refactoring that might be controversial or need more careful review)
There was a problem hiding this comment.
A clear docstring fix is usually a fast merge,
Experience suggests otherwise. I'll fix this downstream and leave this for the next sucker.
See #4622 and #1720 (comment)
This removes
pinvfrom the initialize step, which saves time on large(ish) exogs.