Vasilis/docs #370

vsyrgkanis · 2021-01-14T16:22:44Z

changing the doc structure

…ests

kbattocchi

Shouldn't there be many more notebook changes to move everything to the new names? Also found a few minor issues.

kbattocchi · 2021-01-15T21:28:32Z

econml/bootstrap.py

+            "intervals, consider passing `inference='bootstrap'` or "
+            "`inference=econml.inference.BootstrapInference(n_bootstrap_samples=..., bootstrap_type=...)`, "
+            "as a keyword argument at the `fit` method of the CATE estimator.")
+class BootstrapEstimator(bootstrap.BootstrapEstimator):


I thought you had previously mentioned liking the ease of use for wrapping sklearn estimators as well - do we really want to deprecate that? It seems like a generally useful utility.

We'll still maintain the estimator in the code. But I think it's important for the users of the econml library to know that this is not the way to use the estimator with our cate estimates and that they shouldn't be using this class directly (we already had some users face this). So I do think we should make it a private class. One can still access it as econml.inference._bootstrap.BootstrapEstimator

econml/deepiv.py

econml/drlearner.py

kbattocchi · 2021-01-15T22:00:17Z

econml/sklearn_extensions/linear_model.py

+    .. testcode::
+        :hide:
+
+        # Our classes that derive from sklearn ones sometimes include
+        # inherited docstrings that have embedded doctests; we need the following imports
+        # so that they don't break.
+
+        import numpy as np
+        from sklearn.linear_model import lasso_path
+


I think these should only need to be imported once.

Suggested change

.. testcode::

:hide:

# Our classes that derive from sklearn ones sometimes include

# inherited docstrings that have embedded doctests; we need the following imports

# so that they don't break.

import numpy as np

from sklearn.linear_model import lasso_path

Unforutnately now that each class is in a separate autosummary page, we need this to be on each page. We can no longer just put it at the top of the file for instance.

kbattocchi · 2021-01-15T22:00:41Z

econml/sklearn_extensions/linear_model.py

+    .. testcode::
+        :hide:
+
+        # Our classes that derive from sklearn ones sometimes include
+        # inherited docstrings that have embedded doctests; we need the following imports
+        # so that they don't break.
+
+        import numpy as np
+        from sklearn.linear_model import lasso_path
+


Same here.

Suggested change

.. testcode::

:hide:

# Our classes that derive from sklearn ones sometimes include

# inherited docstrings that have embedded doctests; we need the following imports

# so that they don't break.

import numpy as np

from sklearn.linear_model import lasso_path

same as above

kbattocchi · 2021-01-15T22:01:26Z

econml/sklearn_extensions/linear_model.py

+    .. testcode::
+        :hide:
+
+        # Our classes that derive from sklearn ones sometimes include
+        # inherited docstrings that have embedded doctests; we need the following imports
+        # so that they don't break.
+
+        import numpy as np
+        from sklearn.linear_model import lasso_path
+


And here.

Suggested change

.. testcode::

:hide:

# Our classes that derive from sklearn ones sometimes include

# inherited docstrings that have embedded doctests; we need the following imports

# so that they don't break.

import numpy as np

from sklearn.linear_model import lasso_path

same as above

kbattocchi · 2021-01-15T22:01:51Z

econml/sklearn_extensions/linear_model.py

+    .. testcode::
+        :hide:
+
+        # Our classes that derive from sklearn ones sometimes include
+        # inherited docstrings that have embedded doctests; we need the following imports
+        # so that they don't break.
+
+        import numpy as np
+        from sklearn.linear_model import lasso_path
+


And here

Suggested change

.. testcode::

:hide:

# Our classes that derive from sklearn ones sometimes include

# inherited docstrings that have embedded doctests; we need the following imports

# so that they don't break.

import numpy as np

from sklearn.linear_model import lasso_path

same as above

econml/iv/tsls/__init__.py

vsyrgkanis · 2021-01-16T01:01:47Z

Shouldn't there be many more notebook changes to move everything to the new names? Also found a few minor issues.

Most of the actual import paths did not change because the name of the folder is rhe name of the previous file and all public classes of each folder are included in the init

kbattocchi · 2021-01-19T13:43:22Z

econml/tests/test_bootstrap.py

@@ -292,7 +292,7 @@ def test_stratify_orthoiv(self):
        X = np.array([1, 1, 2, 2, 1, 2, 1, 2]).reshape(-1, 1)
        est = LinearIntentToTreatDRIV(model_Y_X=LinearRegression(), model_T_XZ=LogisticRegression(),
                                      flexible_model_effect=LinearRegression(), cv=2)
-        inference = BootstrapInference(n_bootstrap_samples=20)
+        inference = BootstrapInference(n_bootstrap_samples=20, n_jobs=-1, verbose=3)


Should we be testing that the output is more verbose in this case?

Not sure how to do that easily. I’d say lets postpone

vasilismsr and others added 23 commits January 12, 2021 22:09

re-structured public module in docs

05d824a

re-structured public module in docs

d7abbcc

fixed map and comparison table

4134e8f

fixed inference test

7987042

Merge branch 'master' into vasilis/docs

b9cfdf4

moved orf to folder. moved automl to folder. changed docs. fixed doct…

c774ab9

…ests

added deprecated orf

fe1112e

doc fixes

427aedb

auto init missing

77db23e

small fixes

b435131

removed explicit automl docs until azureml fix

f7629d7

removed automl for now from docs until import is fixed

c16e424

restructured to folders

7ccaa8d

added iv/nnet init

c1c4159

fixed import

34289a7

fixed relative importst

7092741

fixed bootstrap relative imports

95edf8f

fixed relative imports

f3b0033

fxed imports

d93c188

lasso doctest

3c126a9

doctest in ofrest

d5d3f2c

changed to NonParamTSLS

f02978e

added more text to bootstrap deprecation warning

8ea3976

vsyrgkanis requested review from kbattocchi and moprescu January 15, 2021 16:21

vsyrgkanis added the enhancement New feature or request label Jan 15, 2021

vsyrgkanis marked this pull request as ready for review January 15, 2021 16:21

kbattocchi requested changes Jan 15, 2021

View reviewed changes

fixed review feedback

dba96ad

vasilismsr and others added 15 commits January 15, 2021 20:47

merged with master

0ba268f

merge master

fae4f01

changed to particular shap commit

04f0afb

reverted shap and added intersphinx

d2ecaf8

reverted to commit shap

f160711

fixed wording in hyperparm tuning

d5e61bc

Merge branch 'master' into vasilis/docs

e36347e

fixed reference to hoenst forest in docs

7795d80

moved tsls under sieve submodule

80781e8

changed tsls to sieve.SieveTSLS

2bdbb37

fixed azure pipes

66729bc

fixed azure pipes

352772b

added verbosity to bootstrap

cfc93f2

mvoed bootstrap to private

70337ce

made deprecation changes

4b8730e

kbattocchi approved these changes Jan 19, 2021

View reviewed changes

vsyrgkanis merged commit a27fa3c into master Jan 19, 2021

vsyrgkanis deleted the vasilis/docs branch January 19, 2021 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vasilis/docs #370

Vasilis/docs #370

vsyrgkanis commented Jan 14, 2021

kbattocchi left a comment

kbattocchi Jan 15, 2021

vsyrgkanis Jan 15, 2021

kbattocchi Jan 15, 2021

vsyrgkanis Jan 15, 2021

kbattocchi Jan 15, 2021

vsyrgkanis Jan 15, 2021

kbattocchi Jan 15, 2021

vsyrgkanis Jan 15, 2021

kbattocchi Jan 15, 2021

vsyrgkanis Jan 15, 2021

vsyrgkanis commented Jan 16, 2021

kbattocchi Jan 19, 2021

vsyrgkanis Jan 19, 2021

Vasilis/docs #370

Vasilis/docs #370

Conversation

vsyrgkanis commented Jan 14, 2021

kbattocchi 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

vsyrgkanis commented Jan 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment