Suggested Updates to PR #10
Conversation
|
I think some resid/fittedvalues methods in tsa may also be made redundant, am holding off on those for now. |
| @@ -973,10 +974,13 @@ def _predict(self, h=None, smoothing_level=None, smoothing_slope=None, | |||
| formatted = formatted.loc[included] | |||
|
|
|||
| hwfit = HoltWintersResults(self, self.params, fittedfcast=fitted, | |||
| _fittedvalues=fitted[:-h - 1], fcastvalues=fitted[-h - 1:], | |||
| sse=sse, level=level, slope=slope, season=season, aic=aic, | |||
| fittedvalues=fitted[:-h - 1], | |||
There was a problem hiding this comment.
AFAIK, this doesn't work
we cannot override a cached property by an attribute
There was a problem hiding this comment.
In __init__ this now explicitly sets self._fittedvalues = fittedvalues
There was a problem hiding this comment.
Does this also resolve your comment above in __init__?
| @@ -304,8 +304,9 @@ class HoltWintersResults(Results): | |||
| summary | |||
| """ | |||
|
|
|||
| def __init__(self, model, params, **kwargs): | |||
| def __init__(self, model, params, fittedvalues=None, **kwargs): | |||
There was a problem hiding this comment.
I'm not sure about this
in the standard pattern
fittedvalues should not be an explicit argument, because it should be a results cached attribute
in here it might be different because fittedvalues similar to statespace models is a byproduct of the estimation.
|
overall it looks fine in quick skimming |
| @@ -1621,12 +1621,6 @@ class IVGMMResults(GMMResults): | |||
| def fittedvalues(self): | |||
| return self.model.predict(self.params) | |||
There was a problem hiding this comment.
I think this can also go, but it isn't 100% obvious that this is equivalent to self.predict().
|
@josef-pkt any more thoughts on this? I'm pretty stoked to see these tests fixed up. |
|
main question now is whether I really want to add all this to my fittedvalues PR. I like a PR to stay focused on a specific issue, and minimal surrounding changes. |
|
updated with requested edits. |
|
Any thoughts? |
|
@josef-pkt wadaya say? it'd be nice to get this off my desktop |
|
@josef-pkt ping; any thoughts on this? |
|
Any chance of this going anywhere? Is there a subset that could go through quickly? |
|
@josef-pkt any thoughts on how to proceed here? |
The removing of now-inherited methods is tentative, could use another pair of eyeballs (and CI).
The cleanups are only in code that is already touched by your PR, so shouldn't unduly affect the git history.