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

Added Hmine algorithm #1020

Merged
merged 10 commits into from
Mar 28, 2023
Merged

Added Hmine algorithm #1020

merged 10 commits into from
Mar 28, 2023

Conversation

fatihsen20
Copy link
Contributor

@fatihsen20 fatihsen20 commented Mar 23, 2023

Code of Conduct

New feature.

Description

In order to enrich the library and to compare the speed and memory costs of certain algorithms, I added the hmine algorithm to the library.

Related issues or pull requests

None

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

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

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.

Awesome, thanks so much for the contribution!

H-Mine is guaranteed to find the same frequent itemsets as apriori and fp-growth, correct? Would it be possible to add a unit test to test for equivalence with the existing algos?

@@ -931,4 +931,11 @@ imput arrays via `transform` and `fit_transform`

### Version 0.1.1 (2014-08-13)
Copy link
Owner

Choose a reason for hiding this comment

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

I think you accidentally added this to the wrong version in the Changelog

@rasbt
Copy link
Owner

rasbt commented Mar 23, 2023

PS: Don't worry about the linter above, we can address that (happy to take care of that) at the very end.

@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Patch coverage: 90.59% and project coverage change: +0.13 🎉

Comparison is base (11b61b9) 77.33% compared to head (21e038d) 77.46%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1020      +/-   ##
==========================================
+ Coverage   77.33%   77.46%   +0.13%     
==========================================
  Files         198      200       +2     
  Lines       11171    11287     +116     
  Branches     1461     1480      +19     
==========================================
+ Hits         8639     8744     +105     
- Misses       2318     2324       +6     
- Partials      214      219       +5     
Impacted Files Coverage Δ
mlxtend/frequent_patterns/tests/test_hmine.py 88.46% <88.46%> (ø)
mlxtend/frequent_patterns/hmine.py 92.06% <92.06%> (ø)
mlxtend/frequent_patterns/__init__.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fatihsen20 fatihsen20 requested a review from rasbt March 24, 2023 21:46
@rasbt
Copy link
Owner

rasbt commented Mar 24, 2023

Thanks a lot for the update! I will do my best to review it this weekend!

@fatihsen20
Copy link
Contributor Author

I always overlook the isort rules. I am sorry 🥲

@rasbt
Copy link
Owner

rasbt commented Mar 25, 2023

No worries about that! The style checks are really not a big deal, we can address those at the very end upon merging.

@fatihsen20
Copy link
Contributor Author

Hello, will my pull request be merge? I am very excited about this 😊 @rasbt

@rasbt
Copy link
Owner

rasbt commented Mar 27, 2023

This is a fantastic PR! And sorry, will merge soon! Haven't had a chance to go over the docs yet! I have a question here:
Screenshot 2023-03-27 at 10 44 12 AM

You mentioned

on a large dataset.

but did you mean

on a small dataset.

I.e., the dataset from the example above? No worries about fixing it. I did some other tweaks in the docs and will update soon (pls don't modify the Nb in the meantime due to merge conflicts, Jupyter NB's are still a bit tricky on GitHub 😅)

@fatihsen20
Copy link
Contributor Author

This is a fantastic PR! And sorry, will merge soon! Haven't had a chance to go over the docs yet! I have a question here: Screenshot 2023-03-27 at 10 44 12 AM

You mentioned

on a large dataset.

but did you mean

on a small dataset.

I.e., the dataset from the example above? No worries about fixing it. I did some other tweaks in the docs and will update soon (pls don't modify the Nb in the meantime due to merge conflicts, Jupyter NB's are still a bit tricky on GitHub 😅)

Sorry, I probably missed it while preparing the document. Yes, there will be a small dataset in the example there. Thank you so much for your hard work 😊

@rasbt
Copy link
Owner

rasbt commented Mar 27, 2023

Oh I thought it was based on the small example at the top. So you ran the benchmark on a small hands-on dataset if I understand it correctly? I think we can leave things as is unless it's not too large and we can add it to the doc repo. What do you think?

@fatihsen20
Copy link
Contributor Author

Oh I thought it was based on the small example at the top. So you ran the benchmark on a small hands-on dataset if I understand it correctly? I think we can leave things as is unless it's not too large and we can add it to the doc repo. What do you think?

Yes it was based on the small example above. Since we are testing with small dataset on Doc, it may remain small. I don't think it will be a big problem if we add it this way.

@rasbt
Copy link
Owner

rasbt commented Mar 27, 2023

Ok perfect, then I'd say it's fine as is because the example is already in the notebook.

Should be good to merge then, correct?

@fatihsen20
Copy link
Contributor Author

Ok perfect, then I'd say it's fine as is because the example is already in the notebook.

Should be good to merge then, correct?

Okey sir, please 😊

@rasbt rasbt merged commit 54a6bb6 into rasbt:master Mar 28, 2023
@rasbt
Copy link
Owner

rasbt commented Mar 28, 2023

Merged it @fatihsen20 . Thanks again for this awesome PR!
I am hoping to make a new release version on the upcoming weekend

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.

2 participants