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}
SvenKlaassen
left a comment
SvenKlaassen
left a comment
There was a problem hiding this comment.
Thanks. Looks already quite great.
Can you further add two test files test_model_defaults.py and test_return_types.py to the plm submodule similar to (https://github.com/DoubleML/doubleml-for-py/blob/main/doubleml/did/tests/test_return_types.py and https://github.com/DoubleML/doubleml-for-py/blob/main/doubleml/did/tests/test_model_defaults.py).
Only add the corresponding tests for DoubleMLLPLR and I can move the tests from the other classes from the tests directory later.
| """ | ||
| return self._learner | ||
|
|
||
| @property |
There was a problem hiding this comment.
I think it is a good idea to include a more obvious property for the name, but this should be identical to params_names right?
Especially how it is handled in evaluate_learners(). So do we need a extra list?
|
|
||
| # nuisance m | ||
| if m_external: | ||
| m_hat = {"preds": external_predictions["ml_m"], "targets": None, "models": None} |
There was a problem hiding this comment.
For the elements where it is possible can you add the targets to the external predicitons (as in
doubleml-for-py/doubleml/irm/apo.py
Line 261 in 99e4116
evaluate_learners() with external predicitons (available for all new methods).
There was a problem hiding this comment.
Added except for ml_t, which is fold specific
| def _nuisance_tuning( | ||
| self, smpls, param_grids, scoring_methods, n_folds_tune, n_jobs_cv, search_mode, n_iter_randomized_search | ||
| ): | ||
| if self._i_rep is None: |
There was a problem hiding this comment.
Especially since we would like to remove fold-specific hyperparameters.
Addition of logistic partially linear model.
Created PR to check for merging and review changes. Unit tests in work.