Skip to content
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

Add Jaccard, Certainty, and Kulczynski association rules metrics #1099

Merged
merged 18 commits into from
Jul 2, 2024

Conversation

UltraArceus3
Copy link
Contributor

Description

We have implemented three association rules metrics: Jaccard Coefficient, Certainty Factor, and Kulczynski.
We have added appropriate test cases for each of these metrics.
We have also added an optional parameter to association_rules, called return_metrics. The user can pass in a list of metrics for the function to return, rather than having the function generate every implemented metric.

Related issues or pull requests

Resolves #1096

Pull Request Checklist

  • Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file (if applicable)
  • Added appropriate unit test functions in the ./mlxtend/*/tests directories (if applicable)
  • Modify documentation in the corresponding Jupyter Notebook under mlxtend/docs/sources/ (if applicable)
  • Ran PYTHONPATH='.' pytest ./mlxtend -sv and make sure that all unit tests pass (for small modifications, it might be sufficient to only run the specific test file, e.g., PYTHONPATH='.' pytest ./mlxtend/classifier/tests/test_stacking_cv_classifier.py -sv)
  • Checked for style issues by running flake8 ./mlxtend

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@UltraArceus3
Copy link
Contributor Author

Hi @rasbt, we're looking at the failed formatting test case. The suggested formatting seems to be strange.

image

It looks like it is taking issue with two lines in test_association_rules.py. It is formatting just those two lines rather than the entire block. We could go back and change all the lines to reflect that same format, but then it would take up ~100 lines of just numbers. What are your thoughts?

Copy link
Owner

@rasbt rasbt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise, this looks pretty good to me!

docs/sources/CHANGELOG.md Outdated Show resolved Hide resolved
mlxtend/frequent_patterns/association_rules.py Outdated Show resolved Hide resolved
@rasbt
Copy link
Owner

rasbt commented Jul 2, 2024

It looks like it is taking issue with two lines in test_association_rules.py. It is formatting just those two lines rather than the entire block. We could go back and change all the lines to reflect that same format, but then it would take up ~100 lines of just numbers. What are your thoughts?

Thanks for the PR. I am totally fine with not changing the formatting as suggested by black here. Let's see if this can be achieved via an # fmt: off

Copy link
Owner

@rasbt rasbt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the small formatting modifications! Thanks again for the PR, looks really good!

@Nachiket18
Copy link
Contributor

Thanks @rasbt for reviewing and approving the change. It was fun to contribute to the library!

@ankithn30
Copy link
Contributor

ankithn30 commented Jul 2, 2024

Thanks @rasbt for approving and reviewing our change! It was a great experience to contribute to this library!

@UltraArceus3
Copy link
Contributor Author

Thank you @rasbt! It was great working with you on this change. We hope to continue contributing to your library in the future!

@rasbt rasbt merged commit d9713ea into rasbt:master Jul 2, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding more metrics for Association Rules
4 participants