{"action":"created","issue":{"url":"https://api.github.com/repos/statsmodels/statsmodels/issues/3734","repository_url":"https://api.github.com/repos/statsmodels/statsmodels","labels_url":"https://api.github.com/repos/statsmodels/statsmodels/issues/3734/labels{/name}","comments_url":"https://api.github.com/repos/statsmodels/statsmodels/issues/3734/comments","events_url":"https://api.github.com/repos/statsmodels/statsmodels/issues/3734/events","html_url":"https://github.com/statsmodels/statsmodels/issues/3734","id":233012357,"number":3734,"title":"BUG: mutable model.df_model in discrete_model","user":{"login":"jbrockmendel","id":8078968,"avatar_url":"https://avatars2.githubusercontent.com/u/8078968?v=3","gravatar_id":"","url":"https://api.github.com/users/jbrockmendel","html_url":"https://github.com/jbrockmendel","followers_url":"https://api.github.com/users/jbrockmendel/followers","following_url":"https://api.github.com/users/jbrockmendel/following{/other_user}","gists_url":"https://api.github.com/users/jbrockmendel/gists{/gist_id}","starred_url":"https://api.github.com/users/jbrockmendel/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jbrockmendel/subscriptions","organizations_url":"https://api.github.com/users/jbrockmendel/orgs","repos_url":"https://api.github.com/users/jbrockmendel/repos","events_url":"https://api.github.com/users/jbrockmendel/events{/privacy}","received_events_url":"https://api.github.com/users/jbrockmendel/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2017-06-01T21:13:47Z","updated_at":"2017-06-01T23:41:54Z","closed_at":null,"body":"Motivated by #3727:\r\n\r\n> The k_extra attribute is for some generic, inherited methods, I don't remember which.\r\n\r\n`k_extra` modifies `FooResults.df_model` and `df_resid`, but handling `k_extra` in a consistent manner requires a bit of kludge-work.  In the process of writing out _why_ it requires kludge-work, I found that a pet peeve was actually a bug.\r\n\r\nEach of the `L1FooResults` classes alter `model.df_model` and `df_resid` within their `__init__` methods.  I think this should be avoided, as `model.df_foo` should be essentially immutable.  The problem caused by mutability is this:\r\n\r\n```\r\n>>> data = sm.datasets.randhie.load()\r\n>>> mod = sm.NegativeBinomial(data.endog, data.exog, loglike_method='nb2')\r\n>>> res1 = model.fit()\r\n>>> res2 = model.fit_regularized(alpha=100)\r\n>>> res3 = model.fit()\r\n>>> res1.df_model\r\n8.0\r\n>>> res3.df_model\r\n6\r\n>>> (res1.aic, res3.aic)\r\n(87507.710855394151, 87503.710855394151)\r\n```\r\n\r\n`res1` and `res3` should be identical, but are not because the L1FooResults instance is modifying model.df_bar.  \r\n\r\nIn my local \"screw backward compatibility\" branch, the closest thing I've found to a solution is to define the Results-level `k_extra`, `df_model`, and `df_resid` as properties/cache_readonlys that get mixed in to DiscreteResults:\r\n\r\n```\r\nclass DiscreteResultsFreedomMixin(object):\r\n\r\n\t@cache_readonly\r\n\tdef k_extra(self):\r\n\t\tmodel = self.model\r\n\t\tk_extra = model.k_extra\r\n\t\tif getattr(model, 'loglike_method', '').startswith('nb'): ## FIXME: kludge\r\n\t\t\tk_extra -= 1\r\n\t\treturn k_extra\r\n\r\n\t@cache_readonly\r\n\tdef df_model(self):\r\n\t\tk_constr = self.k_constr\r\n\t\tk_extra = self.k_extra\r\n\r\n\t\tret = self.model.df_model - (k_constr + k_extra)\r\n\t\treturn ret\r\n\r\n\t@cache_readonly\r\n\tdef df_resid(self):\r\n\t\tk_constr = self.k_constr\r\n\t\tk_extra = self.k_extra\r\n\r\n\t\tret = self.model.df_resid + (k_constr + k_extra)\r\n\t\treturn ret\r\n```\r\n\r\nThis avoids the need to alter model.df_foo (which gets set only in `DiscreteModel.initialize` and `MultinomialModel.initialize`), but I'm not wild about the kludge that sets `k_extra`, in part because I'm not sure it is generally-correct as opposed to doesn't-cause-test-failures-correct.\r\n"},"comment":{"url":"https://api.github.com/repos/statsmodels/statsmodels/issues/comments/305650046","html_url":"https://github.com/statsmodels/statsmodels/issues/3734#issuecomment-305650046","issue_url":"https://api.github.com/repos/statsmodels/statsmodels/issues/3734","id":305650046,"user":{"login":"jbrockmendel","id":8078968,"avatar_url":"https://avatars2.githubusercontent.com/u/8078968?v=3","gravatar_id":"","url":"https://api.github.com/users/jbrockmendel","html_url":"https://github.com/jbrockmendel","followers_url":"https://api.github.com/users/jbrockmendel/followers","following_url":"https://api.github.com/users/jbrockmendel/following{/other_user}","gists_url":"https://api.github.com/users/jbrockmendel/gists{/gist_id}","starred_url":"https://api.github.com/users/jbrockmendel/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jbrockmendel/subscriptions","organizations_url":"https://api.github.com/users/jbrockmendel/orgs","repos_url":"https://api.github.com/users/jbrockmendel/repos","events_url":"https://api.github.com/users/jbrockmendel/events{/privacy}","received_events_url":"https://api.github.com/users/jbrockmendel/received_events","type":"User","site_admin":false},"created_at":"2017-06-01T23:41:54Z","updated_at":"2017-06-01T23:41:54Z","body":"I think we're in agreement on the main point regarding the bug: setting `res.df_foo` should not be altering `model.df_foo`.  This can be fixed in a pretty non-invasive way inside discrete_model.  I'll make a PR after wrapping up some day-job responsibilities.\r\n\r\nIn cases where `df_resid_inference` is used, does that affect what the \"correct\" values for `df_model` and `df_resid` are?\r\n\r\n> If constraints are specified in fit_constrained, then the df_xxx also depend on the number of constraints.\r\n\r\nIn the interest of brevity I omitted the mixin class that defines `df_model` and `df_resid` for the `L1BazResults` classes.\r\n\r\n> specific:\r\nwhat is your k_extra? I introduced k_extra exactly to avoid hardcoding models in the generic results methods, i.e. for NegativeBinomial, and GeneralizedPoisson and similar in future.\r\n\r\nOut of personal preference I initialize `k_extra` to 0 in `Model.__init__` and avoid `getattr`.  So `model.k_extra` here is equivalent to `getattr(model, 'k_extra', 0)` in master.  That could have been clearer; my mistake.\r\n\r\nDiscussion of `k_extra` and more generally `df_model` is a big topic on which we already have a few threads going.  My main contentions are a) defining these in fewer places is better than more, b) modifying them in place is undesirable, c) being explicit about the reasoning behind cross-model definitions would be helpful.\r\n\r\n> I still don't think that cached properties give us enough flexibility to override df_xxx in the results instance model or case specific.\r\n\r\nThat's fair.  Would you be more comfortable with a `OneTimeProperty`?  Or the `self._df_model` approach used in #3725?"}}
{"actor":{"display_login":"jbrockmendel"}}