minimal wrapper of MAQ #662

[ras44]

Proposed changes

Added a minimal wrapper for MAQ, a simple test, and dependency on the github repo v0.2.0.

Types of changes

What types of changes does your code introduce to CausalML?
Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Update (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• I have read the CONTRIBUTING doc
• I have signed the CLA
• Lint and unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in downstream modules

Further comments

N/A

[erikcs]

This looks like a very clean and simple integration. Before you lock onto the 0.2.0 tag: I'm happy to make changes downstream if there's anything there you think could be improved. (for example: I could rename the MAQ Python class to MultiQini, that is probably a more informative name)

Also, does copy/pasting the maq intro notebook into the docs here make sense?

[ras44]

thanks @erikcs appreciate the contributions!

Deferring to @t-tte and @jeongyoonlee for comments:

• implementation to inherit docstrings from MAQ
• any upstream change requests for MAQ before creating a version dependency?
• incorporating maq intro notebook

[ras44]

@erikcs if it's ok with you, I thought I might rename the wrapper for get_ipw_scores to something like get_ipw_eval_scores to follow the terminology in the paper. The reason is that there is a function in CausalML called get_simple_iptw which returns IPTW weights. So get_ipw_scores could possibly be misinterpreted as returning the per-arm IPTW weights instead of the per-arm ipw-weighted eval scores.

[erikcs]

Thanks @ras44, I agree, get_ipw_eval_scores sounds more informative! I'd like to make that function rename downstream in the python wrapper too, I think having causalml and the original python wrapper use consistent naming could help reduce potential confusion in the future. On that note, maybe we can decide on MultiQini vs MAQ too? I think a pro of MAQ is that it emphasizes that it is a different animal from "Qini", it's a Qini for many arms, MultiQini might give the impression that it is just many Qini curves?

[ras44]

Good point- I think @t-tte had suggested MultiQini, so perhaps he can provide some input to norm on a naming convention? happy to follow your lead

[t-tte]

Yep, MAQ sounds great to me. I think it's also a good idea to copy paste the notebook. As regards inheriting the doc strings, if it's technically feasible, I don't think anyone has a philosophical objection against it.

[erikcs]

@ras44 I don't have push access to your branch, but if you add

from .multiqini import MAQ, get_ipw_scores # noqa

to metrics/__init__.py

Then CausaML's documentation builds what you would expect, at least on my laptop: (using maq tag py0.2.2)

I think it's fine just doing

 from maq import MAQ, get_ipw_scores

in metrics/multiqini.py since get_ipw_scores lives in another module from get_simple_iptw?

If I copy the notebook into docs/examples/qini_curves_for_costly_treatment_arms.ipynb and just change the line from maq import MAQ, get_ipw_scores to from causalml.metrics import MAQ, get_ipw_scores, and add this entry to docs/examples.rst then that too works fine here (note though that notebook uses xgboost, is that fine for CausaML online doc build?)

[ras44]

hi @erikcs Thanks for your comments- just getting back to this!

If we opt not to create a wrapper class called MultiQini in multiqini.py (and it does sound like we've decided on following MAQ naming conventions), then could we just add:

from maq import MAQ, get_ipw_scores # noqa

in metrics/__init__.py?

That builds the CausalML documentation as expected on my machine, and avoids creating the additional multiqini.py file entirely.

I haven't tested the notebook yet, but I think the above setup should be able to run the example notebook as you've described, i.e. using from causalml.metrics import MAQ, get_ipw_scores.

Finally, re xgboost in the doc build: xgboost is a dependency, so I think it should be available during the build if I'm understanding correctly:

causalml/pyproject.toml

Line 37 in 06a5f8f

"xgboost",

If that sounds good, I'll add the above import to metrics/__initi__.py and remove the multiqini.py file entirely.

@jeongyoonlee @t-tte

[erikcs]

Thanks @ras44, that sounds great here. I guess another final thing is how to best integrate maq via pyproject.toml, since the package installs from github, a user now needs git and a c++ compiler to install causalml, if that is problematic, it could maybe be added as an optional dependency instead of a dependency?

[ras44]

@erikcs I just added a couple commits and the MAQ import should be all set. All tests are passing. I also copy-pasted the notebook into docs/examples/qini_curves_for_costly_treatment_arms.ipynb.

Re your comments about needing git and a c++ compiler: I think we assume git is installed, but our installation instructions previously included a recommendation and details on how to set up a conda environment with a variety of required system libraries/executables (including a c++ compiler via cxx-compiler):

causalml/README.md

Lines 112 to 120 in 028c63c

	### Create a clean conda environment
	```
	conda create -n causalml-py38 -y python=3.8
	conda activate causalml-py38
	conda install -c conda-forge cxx-compiler
	conda install python-graphviz
	conda install -c conda-forge xorg-libxrender
	```

In a recent rewrite of README.md we intended to move these instructions into the installation section of the docs, but the above seems to have been missed (created #730), so I'll fix that in a new PR. That should ensure that anyone following the instructions has the proper environment set up for the installation/build of MAQ too.

[erikcs]

Thank you @ras44!

[ras44]

@jeongyoonlee @erikcs @t-tte I think we're set to merge, would appreciate any final thoughts!

[jeongyoonlee]

LGTM. Thanks, @ras44 and @erikcs, for the contribution.