Add option for more than one kmeans init to autogmm

[PerifanosPrometheus]

Reference Issues/PRs

Closes #306

What does this implement/fix? Briefly explain your changes.

Adds option to run multiple k-means initializations to autogmm

Any other comments?

• Notebook displaying changes performed

[netlify]

❌ Deploy Preview for graspologic failed.

🔨 Explore the source changes: 0c998d5

🔍 Inspect the deploy log: https://app.netlify.com/sites/graspologic/deploys/61250197d1bec00007f1c1e8

[PerifanosPrometheus]

@tiliu68 I am open to continue working on improving autogmm if you have any other implementations, even simple such as this one in mind.

[tliu68]

@tiliu68 I am open to continue working on improving autogmm if you have any other implementations, even simple such as this one in mind.

Great. Thanks! Any other comments from @bdpedigo? If not, I will update this change into GaussianMixtureIC here

[bdpedigo]

talked to @PerifanosPrometheus yesterday and I don't think this current PR does what we want, as just changing n_init for each GaussianMixture object that we are already fitting won't change anything (I think).

[bdpedigo]

I proposed modifying the param_grid somewhere to add a bunch of n_init=1 GaussianMixture objects without any agglomerative initialization, or something like that

[PerifanosPrometheus]

I proposed modifying the param_grid somewhere to add a bunch of n_init=1 GaussianMixture objects without any agglomerative initialization, or something like that

Just pushed the modified code and it should now only append models ran with multiple k-means initializations with no linkage when n_init is an integer and label_init is None. The param_grid is modified in the process_paramgrid function.

[PerifanosPrometheus]

@tliu68 I think this PR is ready for review! Please let me know if something in my code isn't clear or could be done more efficiently, I am always open to suggestions and improvements.

[PerifanosPrometheus]

@tliu68 I have made some changes requested by Pedigo such as initializing every run with a different seed(even for runs with no multiple initialization), I am now adding rows with n_init: 1. I have yet to remove the redundant run_multiple_init condition as I believe it simplifies reading of the if statement in the process_paramgrid function(in the sense that I think it is much easier to see if run_multiple_init rather than if n_init and label_init is None)but can certainly be removed if requested. Please let me know what you think and if you have suggestions for changing the code.

[PerifanosPrometheus]

@tliu68 I should have addressed the feedback provided, please let me know if you would like me to make some other changes : )

[tliu68]

No more comments from me. BTW, @PerifanosPrometheus I adapted part of your code into our PR to sklearn and added your name and jhu email here (please let me know if it's not correct).

[PerifanosPrometheus]

No more comments from me. BTW, @PerifanosPrometheus I adapted part of your code into our PR to sklearn and added your name and jhu email here (please let me know if it's not correct).

That is correct! Thank you very much for helping me out throughout the whole process! Will update the code to match that in the sklearn PR

[bdpedigo]

@tliu68 if you approve can you approve on github?

[tliu68]

I've changed 'n_init' to 'kmeans_n_init'. And I think the code looks good.

[PerifanosPrometheus]

@tliu68 Thank you very much for everything!

[tliu68]

@tliu68 Thank you very much for everything!

My pleasure!

[bdpedigo]

@tliu68 can you re-review? there are some issues:

1. The code doesn't actually run with kmeans_n_init > 1
2. There are no tests for this new functionality, which I think is why this was not caught.

[PerifanosPrometheus]

@tliu68 can you re-review? there are some issues:

1. The code doesn't actually run with kmeans_n_init > 1
2. There are no tests for this new functionality, which I think is why this was not caught.

Apologies for the inconvenience! Will quickly check the new functionality and add tests

[PerifanosPrometheus]

@Bpedigo Solved the run error. Essentially on the last commits when tliu changed from n_init to kmeans_n_init she forgot that the gmparams appended should not have changed also to kmeans_n_init since GaussianMixture in sklearn only accepts n_init as parameters for kmeans initializations. Would you still like me to add some tests? If so what type of test could I add?

[bdpedigo]

@PerifanosPrometheus I'd just like a test showing that the code runs basically. Can copy one of the simple tests that are already there and just change that one parameter to kmeans_n_init=2 or something

[tliu68]

Sorry, that was my mistake. As for adding new test, maybe we can have something very similar to the test_two_class here and maybe change this line to AutoGMM = AutoGMMCluster(max_components=5, kmeans_n_init=2) and rename the test to something like test_two_class_multiple_kmeans_inits?

[PerifanosPrometheus]

@Bpedigo and @tliu68 will get it done and thank you for the suggestions!

[bdpedigo]

@tliu68 any chance you'd mind resolving conflicts on this, I'm not sure what has changed since this PR. Do you still think it is good to go?

[tliu68]

@tliu68 any chance you'd mind resolving conflicts on this, I'm not sure what has changed since this PR. Do you still think it is good to go?

I believe I've resolved them. And I think it looks good and does what we wanted.

[bdpedigo]

@tliu68 any chance you'd mind resolving conflicts on this, I'm not sure what has changed since this PR. Do you still think it is good to go?

I believe I've resolved them. And I think it looks good and does what we wanted.

Thanks for reviewing @tliu68 and thanks for the PR @PerifanosPrometheus !