-
Notifications
You must be signed in to change notification settings - Fork 850
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
Apriori use sets #393
Apriori use sets #393
Conversation
Thanks for the PR! I think for efficiency reasons, to avoid yet another for-loop, it would be better to do the transformation to sets some place earlier in the code. For example, replacing the line
and then the line for the Would be great if you could also add a short unit test checking that the types of the itemsets is indeed "set" for Thanks! |
Thank you for the advice! I tried to do the transformation earlier in the code on my first go, but I couldn't quite figure out where to do it without messing up the panda data structure. I was able to get it this time with your help though! For the unit test I'm just looping through the returned itemsets and checking the type of each element. I've never had to make unit tests before, but I that test seems to checks what we want. If you think it would be better to do something else though just let me know. |
|
||
|
||
def test_itemsets_type(): | ||
res_colindice = apriori(df, use_colnames=False) # This is defualt behavior |
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.
Could you change "defualt" to "default" ? :)
for i in res_colindice['itemsets']: | ||
assert isinstance(i, set) is True | ||
|
||
res_colnames = apriori(df, use_colnames=True) # This is defualt behavior |
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.
I think this shouldn't be default behavior then, because the line above with "use_colnames=False" is indicated with default behavior
That looks good, thanks! Could you maybe just do the little comment-fix that I mentioned above? Besides that, this looks good! |
Thanks! And yeah, no problem. Sorry about the comments, that was just carelessness on my part. |
That's great, thanks for the fix & PR |
Description
Changes
apriori
to return itemsets as sets instead of lists.Related issues or pull requests
Disscused in issue 344
Pull Request Checklist
./docs/sources/CHANGELOG.md
file (if applicable)./mlxtend/*/tests
directories (if applicable)mlxtend/docs/sources/
(if applicable)nosetests ./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.,nosetests ./mlxtend/classifier/tests/test_stacking_cv_classifier.py -sv
)flake8 ./mlxtend