Global Refactoring #92

brash6 · 2024-08-01T16:27:27Z

I am opening this PR so that you can start to see the work I have done until now.
This PR is still messy and not clean.

What I have done (a lot is inspired from the work initiated by @houssamzenati and what we have defined in the issue #90 :

Modularize estimators
Add nuisances functions in a nuisances folder / Add useful utils functions
Create an experiment.py file to test modularized estimators with the data generation from med_bench

TO DO :

Ensure every estimator is working as expected
Explicitly decide which set of parameters to be used in every scenario (data generation, classifiers, regressors, every other hyper parameters)
Create tests files covering every defined scenario
Adapt CI to new tests files
Add clean docstrings everywhere
Clean files (remove unused files / functions / variables, enhance / unify naming, strongtyping, etc.)
Add an example file in which each estimator is run

FYI @judithabk6 @bthirion @houssamzenati

codecov-commenter · 2024-08-01T17:03:03Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 89.22801% with 60 lines in your changes missing coverage. Please review.

Please upload report for BASE (develop@8530e26). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/med_bench/utils/decorators.py	23.68%	29 Missing ⚠️
src/med_bench/estimation/mediation_mr.py	81.53%	12 Missing ⚠️
src/med_bench/estimation/base.py	93.44%	8 Missing ⚠️
src/med_bench/utils/utils.py	73.91%	6 Missing ⚠️
src/med_bench/estimation/mediation_dml.py	91.83%	4 Missing ⚠️
..._bench/estimation/mediation_coefficient_product.py	96.77%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             develop      #92   +/-   ##
==========================================
  Coverage           ?   88.53%           
==========================================
  Files              ?       11           
  Lines              ?      663           
  Branches           ?        0           
==========================================
  Hits               ?      587           
  Misses             ?       76           
  Partials           ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bthirion

Looks like a big step.
I just provided a few thoughts. Hope it helps.

src/med_bench/estimation/base.py

bthirion · 2024-08-08T13:26:23Z

src/med_bench/estimation/base.py

+        """
+        self.rng = np.random.RandomState(123)
+
+        self.mediator_type = mediator_type


curation needed on mediator_type

src/med_bench/estimation/base.py

src/med_bench/estimation/tmle.py

src/med_bench/experiment.py

src/med_bench/nuisances/density.py

judithabk6

additional comment here, there is a whole nuisances folder and a nuisances.py files in utils, should be reorganized.

judithabk6 · 2024-08-22T08:24:58Z

src/med_bench/estimation/dml.py

+
+        self._crossfit = 0
+        self._procedure = procedure
+        self._density_estimation_method = density_estimation_method


seems like density_estimation_method is not used?

judithabk6 · 2024-08-22T08:26:04Z

src/med_bench/estimation/base.py

+    def fit_score_nuisances(self, t, m, x, y, *args, **kwargs):
+        """ Fits the score of the nuisance parameters
+        """
+        clf_param_grid = {}


you can't pass params to the grid then? not sure I get it

judithabk6 · 2024-08-22T08:34:00Z

src/med_bench/estimation/coefficient_product.py

+
+class CoefficientProduct(Estimator):
+
+    def __init__(self, clip : float, trim : float, regularize : bool, **kwargs):


agree they are not needed here

judithabk6 · 2024-08-22T08:38:49Z

src/med_bench/estimation/coefficient_product.py

+            m = m.reshape(-1, 1)
+        if len(t.shape) == 1:
+            t = t.reshape(-1, 1)
+


this should be common to all estimators? or at least a function, like _check_inputs from src/med_bench/utils/utils.py

same for y.ravel()

judithabk6 · 2024-08-22T08:42:42Z

src/med_bench/estimation/dml.py

+        self._clip = clip
+        self._trim = trim
+        self._normalized = normalized
+        self._sample_split = sample_split


parameters needs to be documented

looks like sample_split is not used?

judithabk6 · 2024-08-22T09:06:38Z

src/med_bench/experiment.py

+    coef_prod_estimator.fit(t_train, m_train, x_train, y_train)
+    causal_effects = coef_prod_estimator.estimate(t_test, m_test, x_test, y_test)
+
+    r_risk_score = coef_prod_estimator.score(t_test, m_test, x_test, y_test, causal_effects['total_effect'])


should be implemented in utils maybe, with a metrics module. Just saw that it existed, so need to import from there. Not sure about the naming scores vs metrics

judithabk6 · 2024-08-22T09:08:31Z

src/med_bench/nuisances/cross_conditional_outcome.py

+
+from med_bench.utils.utils import _get_train_test_lists, _get_interactions
+
+def estimate_cross_conditional_mean_outcome_discrete(m, x, y, f, regressors):


not sure about a separate file from estimate_conditional_mean_outcome

Indeed, it would be fine having all nuisances in one module IMHO

judithabk6 · 2024-08-22T09:10:49Z

src/med_bench/nuisances/density.py

+
+    return f_t0, f_t1
+
+def estimate_mediator_probability(t, m, x, y, crossfit, clf_m,


we should harmonize and explain more propensity vs probability vs density

judithabk6 · 2024-08-22T09:13:22Z

src/med_bench/nuisances/utils.py

+        reg = RidgeCV(alphas=alphas, cv=CV_FOLDS)
+    else:
+        reg = RandomForestRegressor(n_estimators=100, min_samples_leaf=10)
+


the objective of the refactor is to generalize to any method beyond ridge and forests, and not hardcode parameters

judithabk6 · 2024-08-22T09:15:39Z

src/med_bench/utils/scores.py

@@ -0,0 +1,45 @@
+import numpy as np


needs docstrings and refs

bthirion

LMK whenever you want a review

brash6 · 2024-10-25T16:18:36Z

LMK whenever you want a review

Thank you @bthirion, probably after our next iteration with @houssamzenati on next Tuesday

This reverts commit 09936d4.

This reverts commit 118a8c3.

brash6 · 2024-11-14T14:39:55Z

@judithabk6 @bthirion
We just addressed all the comments from Judith with @houssamzenati, however, some issues still remain :

GComputation estimator still has large discrepencies with expected values in the tolerance tests : Do we increase the tolerance thresholds or we try to understand the problem deeper ?
TMLE is still not tested
Documentations has to be enhanced (docstrings and online doc)
Some optimizations can still be performed in a further PR (DML removal, some functions merging, etc.)

bthirion · 2024-11-14T21:24:31Z

FOr GComputation, ideally, we should localize the issue. If it takes too long, merge as is but open in parallel an issue together with a code that outllines this issue so that we can investigate it later.

bthirion · 2024-11-14T21:26:45Z

can't we have a test on TMLE similar to that of MultiplyRobust (actually the same test ?)

bthirion · 2024-11-14T21:28:08Z

For code and doc improvements, the good way to proceed is to open a bunch of issues so that we don't lose track of the post-refactoring changes.

judithabk6 · 2024-11-15T09:15:20Z

@bthirion I'm on it, I agree with you that more exploration is necessary to figure out exactly what is going on with Gcomputation, and add tests for TMLE, at least, and improve the docstring and naming of the methods of the base estimator

… tests to debug, and refactor tolerance to make it useful

houssamzenati

The current version seems ready to my mind to be merged. Should we do so and move on to the documentation?

bthirion

LGTM

src/med_bench/estimation/base.py

src/tests/estimation/test_get_estimation.py

bthirion · 2024-12-19T14:02:51Z

The current version seems ready to my mind to be merged. Should we do so and move on to the documentation?

+1 for merging now.

brash6 added 9 commits August 1, 2024 18:17

Modularized estimators

e062cbd

nuisances functions

ea38355

some new constants

6d78313

some new utils functions

124873e

useful decorators

84c185d

useful estimator loader to be uysed in the future

b13147e

still messy nuisances.py file

5e88fc1

some scoring functions

3e4a995

experiment file to try estimators (to be enhanced)

f100116

bthirion reviewed Aug 8, 2024

View reviewed changes

judithabk6 reviewed Aug 22, 2024

View reviewed changes

brash6 added 4 commits October 23, 2024 15:45

remove unused class variables

90b7184

enhance base file

c828524

rename experiment.y into example.py file

5bc00b4

apply refactoring PR comments

81263ee

bthirion reviewed Oct 24, 2024

View reviewed changes

brash6 mentioned this pull request Oct 24, 2024

Get rid of _fit_nuisance and score functions #95

Open

brash6 added 3 commits October 25, 2024 18:16

minor reformat example file

632a094

get rid of _get_interactions in base and add _input_reshape function

8927181

application of modifications

9ad7c0a

brash6 added 8 commits October 29, 2024 17:57

remove cross fitting

48fb5ed

minor changes in coefficient product

d35f7d2

remove discrete procedure in g computation

ab8207a

minor changes in ipw

f2ed579

minor changes in ipw

09936d4

Revert "minor changes in ipw"

9c36a8a

This reverts commit 09936d4.

clean dml

118a8c3

clean dml correct

c14ef7a

This reverts commit 118a8c3.

brash6 added 3 commits November 14, 2024 15:22

tests refactored

825bb12

files cleaning

9bff44b

fix tests

b344910

judithabk6 mentioned this pull request Dec 6, 2024

tests take too long #52

Open

Bertrand Thirion and others added 12 commits December 6, 2024 15:55

started to remove R dependencies

cd93d77

rename methods, clean inputs

6290ddb

tmle testing, exactness tests

a95e610

tmle testing, exactness tests

2789d63

tmle testing, exactness tests

e4f124f

explanations and docstrings TMLE, line formatting

6cc1b66

refactor tests (remove old config parameter and have better names for…

13c591f

… tests to debug, and refactor tolerance to make it useful

remove unused options in get_estimation_results

80f4fa0

direct effect treated, indirect effect control fixes

bc21e83

removed exactness tests, fixed TMLE outputs for tolerance tests

ce48988

slight change in test formatting

5a77e67

remove unused file for the tests

005fb21

houssamzenati reviewed Dec 19, 2024

View reviewed changes

bthirion approved these changes Dec 19, 2024

View reviewed changes

bthirion added 5 commits December 19, 2024 15:00

Update src/med_bench/estimation/base.py

566a08c

Update src/tests/estimation/test_get_estimation.py

b22d50f

Update src/med_bench/estimation/base.py

dce928d

Update src/med_bench/estimation/base.py

dfc0cc0

Update src/med_bench/estimation/base.py

b4a26a9

judithabk6 merged commit 08a997d into develop Jan 15, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global Refactoring #92

Global Refactoring #92

brash6 commented Aug 1, 2024 •

edited

Loading

codecov-commenter commented Aug 1, 2024 •

edited

Loading

bthirion left a comment

bthirion Aug 8, 2024

judithabk6 left a comment

judithabk6 Aug 22, 2024

judithabk6 Aug 22, 2024

judithabk6 Aug 22, 2024

judithabk6 Aug 22, 2024

judithabk6 Aug 22, 2024

judithabk6 Aug 22, 2024

judithabk6 Aug 22, 2024

judithabk6 Aug 22, 2024

judithabk6 Aug 22, 2024

bthirion Nov 3, 2024

judithabk6 Aug 22, 2024

judithabk6 Aug 22, 2024

judithabk6 Aug 22, 2024

bthirion left a comment

brash6 commented Oct 25, 2024

brash6 commented Nov 14, 2024

bthirion commented Nov 14, 2024

bthirion commented Nov 14, 2024

bthirion commented Nov 14, 2024

judithabk6 commented Nov 15, 2024

houssamzenati left a comment

bthirion left a comment

bthirion commented Dec 19, 2024


		class CoefficientProduct(Estimator):

		def __init__(self, clip : float, trim : float, regularize : bool, **kwargs):


		from med_bench.utils.utils import _get_train_test_lists, _get_interactions

		def estimate_cross_conditional_mean_outcome_discrete(m, x, y, f, regressors):


		return f_t0, f_t1

		def estimate_mediator_probability(t, m, x, y, crossfit, clf_m,

Global Refactoring #92

Global Refactoring #92

Conversation

brash6 commented Aug 1, 2024 • edited Loading

codecov-commenter commented Aug 1, 2024 • edited Loading

Codecov Report

bthirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judithabk6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

brash6 commented Oct 25, 2024

brash6 commented Nov 14, 2024

bthirion commented Nov 14, 2024

bthirion commented Nov 14, 2024

bthirion commented Nov 14, 2024

judithabk6 commented Nov 15, 2024

houssamzenati left a comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

bthirion commented Dec 19, 2024

brash6 commented Aug 1, 2024 •

edited

Loading

codecov-commenter commented Aug 1, 2024 •

edited

Loading