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.

Sign up for GitHub

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

Jump to bottom

Fairadapt inclusion in AIF360 #257

Merged

hoffmansc merged 15 commits into Trusted-AI:master from dplecko:fairadapt_to_aif

Jan 20, 2022

Contributor

dplecko commented Jul 28, 2021

Hi guys,

Here is a pull request adding three files we mentioned (fairadapt.py, test_fairadapt.py and demo_fairadapt.ipynb).

Most importantly: the installation of the necessary R-packages using rpy2 is currently done from the fairadapt class (check fairadapt.py line 54). I am guessing you would be looking for a better solution, i.e., that R-packages are installed together with AIF. Please just make sure, if this option is used, that we have installed:

ranger version 0.13.1
fairadapt version 0.2.0

Both of the above are now available via CRAN, which should make installing them using conda much easier!

Thank you in advance for attending to this!

Best,
Drago

lgtm-com bot commented Jul 28, 2021

This pull request introduces 13 alerts when merging 3ff929e into fcda24e - view on LGTM.com

new alerts:

12 for Unused import
1 for Module is imported more than once

nrkarthikeyan self-requested a review

October 29, 2021 15:34

Collaborator

nrkarthikeyan commented Oct 29, 2021

Hi @dplecko - can you check the LGTM report above and remove unused imports?

nrkarthikeyan requested changes

View reviewed changes

Collaborator

nrkarthikeyan left a comment

Looks good overall. Some changes requested. Thanks for the contribution.

please also modify README to include this method and setup.py (if needed). Can you also run sphinx and check if the documentation compiles properly?

aif360/algorithms/preprocessing/fairadapt.py Outdated

+              		# selectively install the missing packages
+              		names_to_install = [x for x in packnames if not robjects.packages.isinstalled(x)]
+              		if len(names_to_install) > 0:
+              			utils.install_packages(StrVector(names_to_install))

Collaborator

nrkarthikeyan Oct 29, 2021

Can you install this through modification of setup.py as a dependency of fairadapt? Best place is to do this in the extras block and define a new key for fairadapt. If that is not possible since these are r packages, its ok to have them here.

Collaborator

nrkarthikeyan Oct 29, 2021

What is this utils package BTW? I do not see a function install_packages in utils.py.

Contributor Author

dplecko Nov 4, 2021

this the utils package from base R - I have renamed this now to avoid name clashing.

aif360/algorithms/preprocessing/fairadapt.py Outdated

+              		JMLR paper: https://www.jmlr.org/papers/volume21/19-966/19-966.pdf [1]
+              		Github page: https://github.com/dplecko/fairadapt [2]
+              	"""
+              	def __init__(self,

Collaborator

nrkarthikeyan Oct 29, 2021

Can you also add Attributes here. See https://github.com/Trusted-AI/AIF360/blob/master/aif360/sklearn/postprocessing/calibrated_equalized_odds.py for an example. Attributes are public attributes of the class.

Contributor Author

dplecko Nov 4, 2021

I added prot_attr_ and groups_ - those two seemed sensible.

aif360/algorithms/preprocessing/fairadapt.py Outdated

		from aif360.sklearn.utils import check_inputs, check_groups


		class fairadapt(BaseEstimator):

Collaborator

nrkarthikeyan Oct 29, 2021

Please follow the standard docstring formatting as given in https://github.com/Trusted-AI/AIF360/blob/master/aif360/sklearn/postprocessing/calibrated_equalized_odds.py to provide references. Also add may be one more sentence about fairadapt to provide more details.

Contributor Author

dplecko Nov 4, 2021

The documentation should now be fixed.

aif360/algorithms/preprocessing/fairadapt.py Outdated

+              		# merge X_train and y_train
+              		df_train = pd.concat([X_train, y_train], axis = 1)
+              		Fairadapt_R = robjects.r(

Collaborator

nrkarthikeyan Oct 29, 2021

Is it necessary to have the R object string here or can we define it down below somewhere and use it here?

Contributor Author

dplecko Nov 4, 2021

I think this is the easiest solution... it could done slightly differently, but would not look much better I believe.

aif360/algorithms/preprocessing/fairadapt.py Outdated

		utils.install_packages(StrVector(names_to_install))


		def fit_transform(self, X_train, y_train, X_test):

Collaborator

nrkarthikeyan Oct 29, 2021

Is it necessary to pass X_train and X_test here? Typically we have a separate predict or transform function to process the test data. If this is how fairadapt works, its fine, just wanted to check.

Contributor Author

dplecko Nov 4, 2021

so the fairadapt workflow does support separating the training and testing; however, when doing so, I would have to be attaching some S3 classes from R to a python object... and would make everything look more wild. I believe this is a good intermediate solution.

examples/sklearn/demo_fairadapt_sklearn.ipynb

		@@ -0,0 +1,424 @@
		{

Collaborator

nrkarthikeyan Oct 29, 2021

Can you include a good preamble in the notebook that describes what fairadapt is, how it works, and also what are the steps in the notebook? The more information the better (to the extent possible). We like to keep the notebooks self-contained so that people can start their exploration here. Also, it is good to say what is unique about fairadapt compared to other preproc methods, and what it can do that other methods cannot. This will help users appreciate the novelty of the method.

Collaborator

nrkarthikeyan Oct 29, 2021

Similar comment applies to individual cells. Please add more information on what each cell does to the extent possible

Contributor Author

dplecko Nov 4, 2021

Ok, this is now expanded with much more detail.

tests/sklearn/test_fairadapt.py

+                  FA = fairadapt.fairadapt(prot_attr = "sex", adj_mat = adj_mat, outcome = "annual-income")
+                  Xf_train, yf_train, Xf_test = FA.fit_transform(X_train, y_train, X_test)
+                  assert isinstance(Xf_train, pd.DataFrame)

Collaborator

nrkarthikeyan Oct 29, 2021

Can you add some metric to test if fairadapt actually worked as expected?

Contributor Author

dplecko Nov 4, 2021

Yep, no problem, I added a check to see that the metric improved.

lgtm-com bot commented Nov 2, 2021

This pull request introduces 13 alerts when merging 6236e0f into a5dac73 - view on LGTM.com

new alerts:

12 for Unused import
1 for Module is imported more than once

lgtm-com bot commented Nov 4, 2021

This pull request introduces 1 alert when merging 580a5a7 into a5dac73 - view on LGTM.com

new alerts:

1 for Unused import

Drago Plecko and others added 5 commits

November 5, 2021 17:40


          Fairadapt code, demo and test

1f1d63a

Signed-off-by: Drago Plecko <www.plecko@gmail.com>


          Fix exponential gradient reduction without protected attribute (Trust…

1fd8e71

…ed-AI#267)

Signed-off-by: Drago Plecko <www.plecko@gmail.com>


          Update component.yaml to kfp v2 sdk

f7dbe9e

Update component.yaml to kfp v2 compatible. In v2,
you need to declare the data type for all of the input/output
arguments.

Signed-off-by: Yihong Wang <yh.wang@ibm.com>
Signed-off-by: Drago Plecko <www.plecko@gmail.com>


          add fairlearn

b4e2b57

Signed-off-by: SSaishruthi <saishruthi.tn@ibm.com>
Signed-off-by: Drago Plecko <www.plecko@gmail.com>


          fix readme; fix docs; add more comments to the notebook; improve the …

59257dd

…testing;

Signed-off-by: Drago Plecko <www.plecko@gmail.com>

dplecko force-pushed the fairadapt_to_aif branch from 580a5a7 to 59257dd Compare

November 5, 2021 21:40

lgtm-com bot commented Nov 5, 2021

This pull request introduces 1 alert when merging 59257dd into a5dac73 - view on LGTM.com

new alerts:

1 for Unused import

nrkarthikeyan approved these changes

View reviewed changes


          Update ci.yml

f3bf417

lgtm-com bot commented Nov 17, 2021

This pull request introduces 1 alert when merging f3bf417 into a5dac73 - view on LGTM.com

new alerts:

1 for Unused import

Collaborator

nrkarthikeyan commented Nov 24, 2021 •

edited

Loading

CI workflows failing at the R interface

@dplecko's analysis:

Fixed several warnings, but the major thing to be solved is the following:
when trying to install fairadapt R-package using rpy2, I come across a problem because the default paths on the machine where the continuous integration is running are not writable
WARNING rpy2.rinterface_lib.callbacks:callbacks.py:123 R[write to console]: 'lib = "/usr/local/lib/R/site-library"' is not writable
for this reason, the installation tries to run interactively and ultimately fails. When running locally, I doubt that this would be a problem.
I was wondering if adding fairadapt to ci.yml would solve the problem? In particular, adding it here:
L81: run: install.packages(c("reticulate", "rstudioapi", "testthat"))
I am wondering if that would help? Not sure if the .libPaths() are shared between rpy2 and default R distribution on the machine.
Alternatively, the package installation could be moved somewhere else. Do you perhaps know how to specify a dependency to an rpy2-R-package directly in setup.py ?

@SSaishruthi @gdequeiroz @hoffmansc - Any thoughts on how to fix this?

Collaborator

hoffmansc commented Dec 7, 2021

Maybe I'm missing something but do we not need to install R first in the build-py section of ci.yml? i.e.,

- name: Set up R
  uses: r-lib/actions/setup-r@v1

dplecko added 2 commits

December 13, 2021 11:52


          Update rpy2 conversion and ci.yml

dd04970

Signed-off-by: Drago Plecko <www.plecko@gmail.com>


          Merge branch 'fairadapt_to_aif' of github.com:dplecko/AIF360 into fai…

2faa21e

…radapt_to_aif

Signed-off-by: Drago Plecko <www.plecko@gmail.com>

dplecko force-pushed the fairadapt_to_aif branch from 277522d to 2faa21e Compare

December 13, 2021 16:55

lgtm-com bot commented Dec 13, 2021

This pull request introduces 1 alert when merging 2faa21e into 963df2e - view on LGTM.com

new alerts:

1 for Unused import


          remove numpy 1.19.5 pin

c74d08f

lgtm-com bot commented Jan 14, 2022

This pull request introduces 1 alert when merging c74d08f into 963df2e - view on LGTM.com

new alerts:

1 for Unused import

hoffmansc added 2 commits

January 14, 2022 10:47


          remove unused imports

1d1e2fc

Signed-off-by: Samuel Hoffman <hoffman.sc@gmail.com>


          deprecate python 3.6 and add 3.9

a75a356

Signed-off-by: Samuel Hoffman <hoffman.sc@gmail.com>

hoffmansc force-pushed the fairadapt_to_aif branch from cca652c to a75a356 Compare

January 14, 2022 15:48

lgtm-com bot commented Jan 14, 2022

This pull request introduces 1 alert when merging a75a356 into 963df2e - view on LGTM.com

new alerts:

1 for Unused import

hoffmansc added 2 commits

January 14, 2022 15:57


          move deps from 'FairAdapt' to 'notebooks'

1cb90b7

added 'plotting' option to igraph

Signed-off-by: Samuel Hoffman <hoffman.sc@gmail.com>


          remove unused imports

b8f809f

Signed-off-by: Samuel Hoffman <hoffman.sc@gmail.com>

lgtm-com bot commented Jan 14, 2022

This pull request introduces 1 alert when merging b8f809f into 963df2e - view on LGTM.com

new alerts:

1 for Unused import

hoffmansc added 2 commits

January 20, 2022 14:09


          remove outcome param and separate R code

740db7a

Signed-off-by: Samuel Hoffman <hoffman.sc@gmail.com>


          adjust versions

17183f3

Signed-off-by: Samuel Hoffman <hoffman.sc@gmail.com>

lgtm-com bot commented Jan 20, 2022

This pull request introduces 2 alerts when merging 17183f3 into 963df2e - view on LGTM.com

new alerts:

2 for Unused import

hoffmansc merged commit bfd0161 into Trusted-AI:master

Illia-Kryvoviaz pushed a commit to Illia-Kryvoviaz/AIF360 that referenced this pull request


          Fairadapt inclusion in AIF360 (Trusted-AI#257)

c71bc0a

* Fairadapt code, demo and test
* remove numpy 1.19.5 pin
* deprecate python 3.6 and support 3.9

Co-authored-by: Drago Plecko <drago.plecko@stat.math.ethz.ch>
Co-authored-by: nrkarthikeyan <knatesan@asu.edu>
Co-authored-by: Samuel Hoffman <hoffman.sc@gmail.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet