-
Notifications
You must be signed in to change notification settings - Fork 763
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
minimal wrapper of MAQ #662 #729
Conversation
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? |
thanks @erikcs appreciate the contributions! Deferring to @t-tte and @jeongyoonlee for comments:
|
@erikcs if it's ok with you, I thought I might rename the wrapper for |
Thanks @ras44, I agree, |
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 |
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. |
@ras44 I don't have push access to your branch, but if you add from .multiqini import MAQ, get_ipw_scores # noqa to Then CausaML's documentation builds what you would expect, at least on my laptop: (using maq tag I think it's fine just doing from maq import MAQ, get_ipw_scores in If I copy the notebook into |
hi @erikcs Thanks for your comments- just getting back to this! If we opt not to create a wrapper class called MultiQini in
in That builds the CausalML documentation as expected on my machine, and avoids creating the additional 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 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: Line 37 in 06a5f8f
If that sounds good, I'll add the above import to |
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? |
@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 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 Lines 112 to 120 in 028c63c
In a recent rewrite of |
Thank you @ras44! |
@jeongyoonlee @erikcs @t-tte I think we're set to merge, would appreciate any final thoughts! |
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.
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 applyChecklist
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.Further comments
N/A