-
Notifications
You must be signed in to change notification settings - Fork 99
LPLR model #365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
LPLR model #365
Conversation
Added test set-up
SvenKlaassen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
doubleml/plm/lplr.py
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially since we would like to remove fold-specific hyperparameters.
I've added them. I had to change the check function a little bit. |
SvenKlaassen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I added skips for the docstring tests and evaluate_learners() to the tests (this needed some update to skip evaluation if targets are not available (for ml_t).
Could you add in comments in the _nuisance_tuning if some approximations are done to allow simple on fold tuning?
Else i only had one other single comment.
| res["preds"][smpls_single_split[1]] += model.predict_proba(x[smpls_single_split[1]])[:, 1] | ||
| else: | ||
| res["preds"][smpls_single_split[1]] += model.predict(x[smpls_single_split[1]]) | ||
| res["preds"] /= len(smpls) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this ?
Addition of logistic partially linear model.
Created PR to check for merging and review changes. Unit tests in work.