LPLR model #365
LPLR model #365
Conversation
Added test set-up
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
doubleml/utils/_estimation.py
Outdated
| n_obs = x.shape[0] | ||
|
|
||
| smpls_is_partition = _check_is_partition(smpls, n_obs) | ||
| # TODO: Better name for smples_is_partition |
There was a problem hiding this comment.
I think this argument is fine. But we might set the default to True
There was a problem hiding this comment.
If we set it to true in its current form it doesn't really work. I've given the argument a new name, but I'm open to better ideas.
There was a problem hiding this comment.
Would it be possible to just se the argument to is_partition with a default of True?
| `sample splitting <https://docs.doubleml.org/stable/guide/resampling.html>`_ in the DoubleML user guide. | ||
| """ | ||
|
|
||
| _double_sample_splitting = False |
There was a problem hiding this comment.
This can be removed right?
doubleml/utils/_estimation.py
Outdated
| n_obs = x.shape[0] | ||
|
|
||
| smpls_is_partition = _check_is_partition(smpls, n_obs) | ||
| # TODO: Better name for smples_is_partition |
There was a problem hiding this comment.
Would it be possible to just se the argument to is_partition with a default of True?
There was a problem hiding this comment.
It would be great to move the dataset tests for the plr models to a separate test_datasets.py file in the plm/tests directory. The same is already done with the did datasets.
| Number of inner folds for nested resampling used internally. | ||
| n_rep : int, default=1 | ||
| Number of repetitions for sample splitting. | ||
| score : {'nuisance_space', 'instrument'} or callable, default='nuisance_space' |
There was a problem hiding this comment.
Callable is not implemented
| super().__init__(obj_dml_data, n_folds, n_rep, score, draw_sample_splitting, double_sample_splitting=True) | ||
|
|
||
| self._error_on_convergence_failure = error_on_convergence_failure | ||
| self._coef_start_val = 1.0 |
|
|
||
|
|
||
| @pytest.mark.ci | ||
| def test_ssm_exception_resampling(): |
|
|
||
| # construct a classifier which is not identifiable as classifier via is_classifier by sklearn | ||
| log_reg = LogisticRegressionManipulatedType() | ||
| # TODO(0.11) can be removed if the sklearn dependency is bumped to 1.6.0 |
There was a problem hiding this comment.
This should not be necessary anymore (dependency is already updated)
| return res_dict | ||
|
|
||
|
|
||
| @pytest.mark.ci |
There was a problem hiding this comment.
Can you also add a test on the se ?
| return self._smpls | ||
|
|
||
| @property | ||
| def smpls_inner(self): |
There was a problem hiding this comment.
Can you add this to the exception tests?
| self : object | ||
| """ | ||
| if self._is_cluster_data: | ||
| if self._double_sample_splitting: |
There was a problem hiding this comment.
Can you add a test_sample_splitting_exceptions.py to test the exceptions from the mixin. In a later update i can move all specific sample splitting exceptions of the DoubleML class there.
Addition of logistic partially linear model.
Created PR to check for merging and review changes. Unit tests in work.