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.

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

Draft implementation of sample_weight for kmodes. #174

Merged
merged 3 commits into from
Mar 30, 2022
Merged

Draft implementation of sample_weight for kmodes. #174

merged 3 commits into from
Mar 30, 2022

Conversation

kklein
Copy link
Contributor

@kklein kklein commented Mar 25, 2022

No description provided.

@coveralls
Copy link

coveralls commented Mar 25, 2022 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 97.925% when pulling 03a92c8 on kklein:kmodes_sample_weight into 4de7bf7 on nicodv:master.

@kklein kklein marked this pull request as ready for review March 25, 2022 17:22
@kklein kklein marked this pull request as draft March 25, 2022 17:52
@kklein
Copy link
Contributor Author

kklein commented Mar 25, 2022

Hi @nicodv ! Unfortunately I'm not able to recreate the test failure locally with python 3.10.4. Locally, both values in question amount to
242.05714285714285 and
242.05714285714288
respectively.

Do you have a hunch on what could be going on here?

@nicodv
Copy link
Owner

nicodv commented Mar 28, 2022

@kklein , I think it would be wise to use a static random seed to all your tests (random_state=42), just like here: https://github.com/nicodv/kmodes/blob/master/kmodes/tests/test_kprototypes.py#L68

I'm hoping it will eliminate any test failures due to random variations in the algorithms.

@kklein kklein marked this pull request as ready for review March 28, 2022 06:54
nicodv
nicodv approved these changes Mar 29, 2022
View reviewed changes
Copy link
Owner

@nicodv nicodv left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for going the full stretch, @kklein !

kmodes/kprototypes.py
@@ -495,17 +495,18 @@ def _k_prototypes_iter(Xnum, Xcat, centroids, cl_attr_sum, cl_memb_sum, cl_attr_
rindx = random_state.choice(choices)

cl_attr_sum, cl_memb_sum = _move_point_num(
Xnum[rindx], old_clust, from_clust, cl_attr_sum, cl_memb_sum
Xnum[rindx], old_clust, from_clust, cl_attr_sum, cl_memb_sum,
Copy link
Owner

Choose a reason for hiding this comment

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

Nice find, small oversight from last PR

@nicodv
Copy link
Owner

nicodv commented Mar 29, 2022

Please merge if you think it's ready, @kklein .

FYI, I'll be making a 0.12.0 release of kmodes after these additions.

@kklein
Copy link
Contributor Author

kklein commented Mar 30, 2022

Thanks for the fast review! :)
I think I don't have permission to merge this but from my side it's good to go!

@nicodv nicodv merged commit 247f193 into nicodv:master Mar 30, 2022
@nicodv nicodv mentioned this pull request Mar 30, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicodv nicodv nicodv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kklein @coveralls @nicodv