Silhouette plot improvements

[neelasha23]

1. Removed figsize parameter from clustering tests and replaced baseline images with correct size.
2. Deprecated silhouette_analysis_from_results function and converted it to an internal function _silhouette_analysis_one_cluster.
3. Removed silhouette_analysis_from_results example from clustering user guide.
4. Modified previous tests related to silhouette_analysis_from_results.

Issue ticket number and link

Closes #150

Checklist before requesting a review

• I have performed a self-review of my code
• I have added thorough tests (when necessary).
• I have added the right documentation (when needed). Product update? If yes, write one line about this update.

---

📚 Documentation preview 📚: https://sklearn-evaluation--213.org.readthedocs.build/en/213/

[coveralls]

Pull Request Test Coverage Report for Build 4009571746

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 28 of 30 (93.33%) changed or added relevant lines in 2 files are covered.
• 3 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.1%) to 94.336%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/sklearn_evaluation/plot/clustering.py	21	23	91.3%

Files with Coverage Reduction	New Missed Lines	%
src/sklearn_evaluation/plot/clustering.py	1	97.58%
src/sklearn_evaluation/plot/precision_recall.py	2	95.92%

Totals
Change from base Build 3989535447:	0.1%
Covered Lines:	2748
Relevant Lines:	2913

---

💛 - Coveralls

[edublancas]

silhouette_analysis_from_results was added to be consistent with elbow_curve_from_results (user wants to generate plots from custom model). Do we need to make elbow_curve_from_results internal too?

the elbow curve plot fits the models internally and generates the plot, but in some cases the user might want to train the model themselves and then pass the data for plotting. at first I thought elbow_curve_from_results was doing something similar but it doesn't like like it (and correct me if I'm wrong)

silhouette_analysis iterates over multiple models (with different num of clusters) and generates one plot per model. however, silhouette_analysis_from_results only takes a single num of clusters value and produces one plot, so they're not exactly a match.

I think it's valuable to have silhouette_analysis_from_results, but then it looks like it'd need to be able to take a list of cluster_labels, because it can only take one at the moment. the idea is that I should be able to take some input data, call silhouette_analysis_from_results and reproduce the exact same plot as silhouette_analysis, but currently, I'd need to wrap silhouette_analysis_from_results in a for loop to achieve so

let me know if this explanation is clear!

[neelasha23]

Yes it's clear. So, basically, for silhouette plot the user would be interested in comparing plots for different values of number of clusters it would be better to have a function that takes a list of values corresponding to multiple n_cluster values. Since silhouette_analysis_from_results is taking one set of values only as input, it wont be of much use to the end user it's better to be made an internal function.
However for elbow plot, the same plot visualises results of a range of the n_cluster value so it would be useful.

In the PR silhouette_analysis_from_results is deprecated and replaced with an internal one.

[edublancas]

took a look at the code.

I think if you move the for loop over range_n_clusters to the from_results version, it'll work as I described right? I think we should keep it. and maybe move the logic that plots a single model one_cluster private.

[neelasha23]

So there'll be two methods now :

1. silhouette_analysis_from_results (public) : will loop over range_n_clusters and generate each plot
2. _silhouette_analysis_one_cluster (private) : this will be internally called from silhouette_analysis_from_results with in the for loop for each value of the range_n_cluster

[edublancas]

correct!

and silhouette_analysis should also call _silhouette_analysis_one_cluster

also, let's rename _silhouette_analysis_one_cluster -> _silhouette_analysis_one_model, it's clearer since the plot is for one clustering model as opposed to a single cluster

[neelasha23]

I have made the changes discussed above.
@edublancas Please let me know if this looks fine

[edublancas]

can you rebase? I've introduced some changes in the docs so the binder links use your code, but I still see the old version.

you can do:

git rebase -i master

# do the rebase process

git push --force

if you need help with the rebase command let me know

[neelasha23]

I did the rebase but I'm not sure if anything got updated. Below is what I see:

Everything up-to-date
(sk-eval) neelashasen@192 sklearn-evaluation % git rebase -i master
Successfully rebased and updated refs/heads/issue_150.
(sk-eval) neelashasen@192 sklearn-evaluation % git push origin pr_curve --force
Everything up-to-date

There weren't any conflicts

[edublancas]

did you pull from master first?

what's the output of this?

git checkout master
git pull

[idomic]

Were we able to solve the issue? Is it ready for merge?

[edublancas]

we were, I'll review the code now

[edublancas]

let's move the _from_results methods (elbow and silhouette) out of this and put them as docstring examples (if we don't have them). then, let's link from here to the docstring so people know they exist. something like:

```{tip}
If you want to train the models yourself, you can use `from_results` to plot.
```

there's a way to link directly to a class definition without a hardcoded link, I think :class:, but I'm unsure if that will work with the markdown format,let's try and see. if it doesn't work, we can use raw rst format: https://jupyterbook.org/en/stable/file-types/restructuredtext.html

[neelasha23]

I think :class: directive works with classes but this one's a function.
I was able to link using heading labels like so

.. _silhouette-analysis-from-results-label:

silhouette_analysis_from_results
--------------------------------
.. autofunction:: sklearn_evaluation.plot.silhouette_analysis_from_results

and then using this label to create a link in the user guide using :ref:. But the tip or note directives are not working along with this reference. Still trying to find the issue

[neelasha23]

Edit: it was an indentation issue. Will push the change

[idomic]

@neelasha23 please fix conflicts

[neelasha23]

Fixed the review comments
@edublancas @idomic

[edublancas]

great! I think there is an equivalent :func: which is preferred over the :ref: approach, so let's use that next time!

[edublancas]

btw, there is no need to include the github link in the changelog, we'll automatically add them with the deployment script