ENH Add post-processing algorithm "Discrimination Aware Decision Tree Learning" by Kamiran et al. #1057
Conversation
There was a problem hiding this comment.
Thanks for the PR @Hadyark .
A few notes:
- Please make sure your lines are not too long, including in the documentation, lines should be <88 chars long.
- The main entry point should be a somewhat scikit-learn compatible estimator, added to the corresponding
__init__.pyfile and exposed publicly. - We follow numpydoc standard for docstrings, please have a look at the existing classes/methods and adapt the docstrings accordingly.
- Your
.ipynbnotebook file should be converted into a.pyfile with the same format as the other examples inexamplesfolder.
I have approved the CI and merging with the latest main should render your documentation for us here.
- implementation - notebook - test
f10bfae to
00c3806
Compare
|
Hi @Hadyark, are you still interested in working on this PR? |
|
@alliesaizan I'm sorry, I forgot about this PR a long time ago. I'll take a look at it this weekend |
|
Hi @Hadyark! I solved the merge conflicts to rebase on main a few days ago and fixed the linting issues to see what the state of the PR is(doing that for all the bigger left over PRs atm), and all tests pass. You are able to pick up with a green CI and address the comments above. Some things have changed since 2022, but the general remarks stay. Let me know if you need any help with the sklearn compatibility! |
Sounds good, just let us know when it's ready for review! |
|
hi @Hadyark I think it would be much easier for you not to bother with the linting issues if you install the pre-commit hooks. Not sure if they were there in the version of the contributing docs a few years ago. From the docs in https://fairlearn.org/v0.12/contributor_guide/development_process.html |
|
When you ask for a scikit-learn-compatible estimator, does this imply having the same parameters as the original methods? For the fit(...) method, I need to add a “sensitive” parameter. Is this a problem? |
|
Hi @Hadyark !
No it is not a problem, provided it is a keyword-argument of the More generally, I believe scikit-learn compatibility is well explained here. And as explained on this section you can check programmatically your estimator's compatibility, via unit tests, using this decorator. You can see an example of how it is used to test the CorrelationRemover's compatibility for instance. I had to make my estimator compatible recently so I might be able to help you. @TamaraAtanasoska is also more familiar with it, she helped me a lot to get it done. |
|
hi @Hadyark, are you still interested in finishing this? should we open this for someone else to pick up in the case you are not? thanks for letting me know in advance! |
|
I'd love to finish this MR, but I can't figure out how to modify my code to make it compatible with scikit-learn. If you have someone in mind to pick up this case, don't hesitate. |
This PR will solve #1024 and implements the Relabeling from the paper Discrimination Aware Decision Tree Learning by Kamiran et al.