Repository navigation
BUG: Validate the method of AbsorbingLS.fit - #783
Merged
Merged
Conversation
AbsorbingLS.fit used LSMR for every name of a method other than "auto" and "hdfe". A misspelled name such as "hdfee", any other name such as "foo", and "HDFE" (which is a natural way to write the name) were accepted without an error or a warning and absorbed the variables using LSMR, which the user had not asked for. The method is now one of "auto", "hdfe" and "lsmr", whatever the case. Any other value, including a value that is not a string, raises a ValueError that names the accepted values and the value that was passed. The name is compared in lower case, so a call with "HDFE" after a call with "hdfe" does not absorb the variables again.
The FutureWarning of AbsorbingLS.fit(lsmr_options=...) said "Use absorb_options". For a model that can be absorbed using HDFE, which is a model without weights whose absorbed variables are all categorical, absorb_options are the options of HDFE unless the method is LSMR, so that advice raises a ValueError (#752 made the options of LSMR with HDFE an error). The warning and the deprecation note in the docstring now say to use absorb_options with method="lsmr", which gives the same results as lsmr_options and does not warn.
…bservations Interaction.drop is public. It removes observations from the instance in place, but nobs kept the number of observations before the call, so an Interaction that had dropped 3 of 50 observations reported 50 although its cat, cont and sparse had 47 rows. AbsorbingLS drops the rows with missing values of its own copies of the interactions in the same way, and so the interactions of a model with missing values reported the number of observations before the rows were dropped. nobs is now the number of observations that remain. drop has a docstring that says that it modifies the instance in place and returns None, and nobs has one too.
The error that clusters have the wrong number of rows is shared by the IV models, AbsorbingLS and the panel covariance estimators. It told users that "the model property `notnull` contains the locations of the observations that have no missing values". The IV models have notnull, but AbsorbingLS has neither notnull nor isnull, and the panel models name the property not_null. The message also said "have have". The message now says what is true of every model: observations with a missing value in the regression variables are dropped when the model is created, so clusters must have one row for each of the remaining observations, and the observations with missing values should be dropped from the data and from clusters before the model is estimated. This is the smaller change. Giving AbsorbingLS a documented notnull and isnull, like the IV models, would still leave the panel models without a property of that name. The first sentence, which the tests match, is unchanged.
The ValueError of IVLIML.fit when kappa cannot be estimated said "This is most likely occurs if the instrument matrix is rank deficient". It now says "This most likely occurs if the instrument matrix is rank deficient". The error of the computation of kappa that follows it is unchanged.
…a period The numpydoc lint of the documentation conventions requires it.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import pytest | ||
|
|
||
| from linearmodels.iv import IV2SLS, IVLIML | ||
| import linearmodels.iv.absorbing |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #783 +/- ##
========================================
Coverage 99.60% 99.60%
========================================
Files 127 129 +2
Lines 23832 24619 +787
Branches 1783 1826 +43
========================================
+ Hits 23737 24521 +784
- Misses 35 36 +1
- Partials 60 62 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AbsorbingLS.fit used LSMR for every name of a method other than "auto" and "hdfe".
A misspelled name such as "hdfee", any other name such as "foo", and "HDFE" (which
is a natural way to write the name) were accepted without an error or a warning and
absorbed the variables using LSMR, which the user had not asked for. The method is
now one of "auto", "hdfe" and "lsmr", whatever the case. Any other value, including
a value that is not a string, raises a ValueError that names the accepted values and
the value that was passed. The name is compared in lower case, so a call with "HDFE"
after a call with "hdfe" does not absorb the variables again.