Improve configuration object for clustering API

[arovir01]

This PR alters the configuration object used in weight clustering in accordance with the RFC proposal. It contains the following changes:

• Added enum CentroidInitialization to capture possible centroid initialization methods
• Configuration object modified to use this enum to specify the centroid initialization technique to be used (instead of the string-based descriptor used until this point)
• Updated serialization and unit tests accordingly

[akarmi]

Thanks @arovir01!

@nutsiepully and @alanchiao, please review this change, introduced following the discussion at the clustering RFC design review.

[alanchiao]

@arovir01

It'd be good to first consider this in the context of something like the feature request template here and more clearly define the motivation.

This change would ultimately expose four more symbols:

1. LinearCentroidsInitialisation
2. RandomCentroidsInitialisation
3. DensityBasedCentroidsInitialisation
4. AbstractCentroidsInitialisation with __init__, get_cluster_centroids and public weights and number_of_clusters attributes that would be subject to backwards-compatibility requirements.

So from the template, what's the motivation of this, relative to the original string-based parameter for configuration?

Has there been explicit interest to extend this list by others, with initialization methods that perform better than before for their use cases? And are there other types of configuration besides initialization that you foresee, which maintaining these new symbols with backwards-compatibility requirements would make harder?

If it's just for experimentation purposes, for now, we can always suggest that people fork the code and modify the implementation of one of the existing initializations to their arbitrary algorithm, to see how it can do better, until there is wider interest. There can even be a Github issue tracking this to wait for people to express interest.

Would an enum be better if there is just an interest to avoid strings?

[arovir01]

@alanchiao

Changed to enum as requested. Is there anything else I should change?

[alanchiao]

API change itself looks good.

1. Is there a way to update CentroidsInitializerFactory so that init_is_supported checks if the init_method is one of the enums, instead of maintaining a second copy of the initializers list?
2. Please update the PR description to reflect the most recent changes. Particularly since it's an API change, the motivation for the change should be clear.

[alanchiao]

Getting another person to also take a look at this and will formalize how many people should generally be reviewing smaller public API changes, according to the ownership RFC, going forward.

[arovir01]

1. Is there a way to update `CentroidsInitializerFactory` so that `init_is_supported` checks if the `init_method` is one of the enums, instead of maintaining a second copy of the initializers list?

We must keep the _initialisers dict in CentroidsInitializerFactory, as that is where the links between the enum values and the implementation classes are defined. I can't really think of a way of doing this differently without exposing the implementation classes in the public API (which would in essence be equivalent to the pre-enum solution from my previous commit).

2. Please update the PR description to reflect the most recent changes. Particularly since it's an API change, the motivation for the change should be clear.

Done.

[alanchiao]

We must keep the _initialisers dict in CentroidsInitializerFactory, as that is where the links between the enum values and the implementation classes are defined. I can't really think of a way of doing this differently without exposing the implementation classes in the public API (which would in essence be equivalent to the pre-enum solution from my previous commit).

Got it.

[nutsiepully]

The change looks good to me on it's own. I don't particularly see a benefit in moving from strings to enums, since python doesn't have much type safety.

But if you prefer enum, that works just as well. Like @alanchiao said, this changes the API surface so just ensure any clients are aware of it.

[alanchiao]

Approving. @arovir01: please take one more look and will merge after you @ me.

[arovir01]

Approving. @arovir01: please take one more look and will merge after you @ me.

@nutsiepully I agree, it is not a huge improvement, but I prefer enums over strings even in Python. I find it a cleaner practice.

@alanchiao I think we can proceed with the merge, there's no further change we wish to make to the code at this stage.