[MRG] Enhance documentation #208
Conversation
wdevazelhes
changed the title
|
I just made a new commit that did the following:
|
|
Maybe also check for broken links to fix #212? |
|
When trying to compile the doc on my local machine it crashes (didn't use to be the case) |
That's right, it worked on my machine but it was because I had skggm installed so I didn't see it: in fact it's because I didn't update |
|
I checked, and with this commit 4b889d4 for lmnn, there is the same problem, so it would mean that the wrong gradient actually generated a prettier picture.. Do you think it's possible ? If so I guess we can use |
There was a problem hiding this comment.
Some quick comments after browsing the doc:
- in Package Overview > Module contents, metric_learn.base_metric is missing a decription
- in the left menu Package Overview > Base classes, it says metric-learn module instead of the corresponding name (base_metric, constraints)
- in the User Guide, the algorithms sections for pairs and quadruplets should appear as subsubsection? (within pair/quadruplet)?
|
Regarding LMNN, can you double check what happens just before the gradient correction? Were we using identity initialization then? |
|
@perimosocordiae any reason why adding |
# Conflicts: # metric_learn/lmnn.py # metric_learn/rca.py
Thanks for the review @bellet, I'll change that |
You mean the correction in #201 ? Yes, it was using identity initialization (see this line: https://github.com/wdevazelhes/metric-learn/blob/69c328cf7e05d374e6d7b19c78f377765752f8c1/metric_learn/lmnn.py#L69)
That's right, will do |
I think this was added because it made the docs look cleaner at the time. If it's nicer with them, then go ahead. |
I see, here is a screenshot of the list of methods it does ("Methods" section), I find it pretty cool: I'll check the whole doc in the end, to see if has bad side effects elsewhere, but if you agree with this, let's go with this flag |
|
Here is the experiment (I noticed that LDA init seemed to work much better) from metric_learn import LMNN
from sklearn.datasets import make_classification
from sklearn.manifold import TSNE
import matplotlib.pyplot as plt
X, y = make_classification(n_samples=100, n_classes=3, n_clusters_per_class=2,
n_informative=3, class_sep=4., n_features=5,
n_redundant=0, shuffle=True, random_state=42)
tsne = TSNE(random_state=42)
X_e = tsne.fit_transform(X)
plt.figure()
plt.scatter(X_e[:, 0], X_e[:, 1], c=y)
plt.show()
lmnn = LMNN(verbose=True, random_state=42)
X_r = lmnn.fit_transform(X, y)
X_er = tsne.fit_transform(X_r)
plt.figure()
plt.scatter(X_er[:, 0], X_er[:, 1], c=y)
plt.show()
last iterations:
details of the cost function In commit d8726d3 (the init is identity): Here is the figure obtained:
And here are the iterations: Note that the fact that the objective function stays constant and the gradient too was a known bug (fixed in #100) that was not fixed at that time. The old LMNN seemed to perform quite well (visually) with respect to new ones, no matter the initialization, but note that the cost function seems better in this PR. I also tried to do less iterations (20) and have a bigger learning (1e-3 instead of 1e-7) rate and it seems to get better visual results (though with a cost function value still high like 1673). I checked the default parameters and they are the same, so it's weird that the behaviour is different... I'll try to understand why |
|
Did you double check that the dataset is exactly the same and that target neighbors and impostors are also the same in each case? If so, the objective function is the same and the current code achieves smaller value so it works better (despite the poorer visualization). Did you try to tweak the parameters of t-SNE (e.g., perplexity) to see whether this is simply a visualization issue? |
Yes, the dataset is the same (I used a random seed and visually it looked the same (TSNE with a random seed too) |
|
I am not sure about that but I think there might be a problem with the impostors: I am not sure yet what the the variable If for instance I "tweak" the margin_radii = 1 + _inplace_paired_L2(Lx[furthest_neighbors], Lx)
margin_radii *= 1000000000It seems to work And it kind of makes sense to me: maybe in |
|
@perimosocordiae @bellet I am not very fast in understanding/parsing the lmnn code, so I wonder, should I continue or should we focus on other things for the release ? |
|
I'd really prefer that we're as confident as we can be in the correctness of our LMNN implementation before we release. Can we compare against the scikit-learn version? |
That makes sense So I guess recomputing the impostors at every step in the TODO at the top of @perimosocordiae @bellet should I begin a PR to do that ? |
|
However, I still don't understand why the number of active constraints was growing at the end of the training... |
|
I'll update the PR #223, where I'll list possible solutions to the LMNN problem, but then in the meantime I'll not deal with this pb in this PR, so that we can merge it as soon as possible |
@bellet I fixed the description (first bullet point), and changed the sections into subsubsections (third bullet point). But for the second bullet point I don't remember if I fixed it already, I think I did, but just to be sure, now in the doc it's written like |
I've just seen, it's when we click on |
…ric-learn into main/improve_doc
|
Since there are a bit of nipticks to change (like uniformize the docstrings by ensuring we say (default=...) after each param, that the doctstring is well indented etc, I'll opened an issue for that, but it's not so urgent so I guess we can wait for the next release for that |
|
Is this ready for review then? |
|
Yes! I guess it is |
|
|
||
| All supervised algorithms are scikit-learn `Estimators`, so they are | ||
| compatible with Pipelining and scikit-learn model selection routines. | ||
| Supervised Metric Learning Algorithms are the easiest metric-learn algorithms |
There was a problem hiding this comment.
I prefer to avoid "easiest" to avoid giving the impression that the other algorithms are complicated to use. Maybe simply "Supervised metric learning algorithms use the same API as scikit-learn"
| .org/stable/glossary.html#term-array-like>`_ objects, `X` and `y`. `X` | ||
| should be a 2D array-like of shape `(n_samples, n_features)`, where | ||
| `n_samples` is the number of points of your dataset and `n_features` is the | ||
| number of attributes of each of your points. `y` should be a 1D array-like |
|
|
||
| .. todo:: Covariance is unsupervised, so its doc should not be here. | ||
| Here is an example of a dataset of two dogs and one | ||
| cat (the classes are 'dog' and 'cat') an animal being being represented by |
|
|
||
| .. note:: | ||
|
|
||
| If the metric learner that you use learns a Mahalanobis Matrix (like it is |
There was a problem hiding this comment.
maybe link to the section in "What is metric learning?"
|
|
||
| All supervised algorithms are scikit-learn `sklearn.base.Estimators`, and | ||
| `sklearn.base.TransformerMixin` so they are compatible with Pipelining and | ||
| scikit-learn model selection routines. |
There was a problem hiding this comment.
maybe name a few sklearn model selection routines to make sure people see what is meant here
| already in the order that points are given in the quadruplet. | ||
|
|
||
|
|
||
| The goal of weakly-supervised metric-learning algorithms is to transform |
There was a problem hiding this comment.
this is irrelevant here
should be replaced by a quick paragraph in the same spirit as for the section on pairs
|
|
||
| .. _quadruplets_predicting: | ||
|
|
||
| Predicting |
| Scoring | ||
| ------- | ||
|
|
||
| Not only are they able to predict the label of given pairs, they can also |
|
|
||
| Not only are they able to predict the label of given pairs, they can also | ||
| return a `decision_function` for a set of pairs. It is basically the "score" | ||
| which sign will be taken to find the prediction for the pair. In fact this |
| of quadruplets have the right predicted ordering. | ||
|
|
||
| >>> lsml.score(quadruplets_test) | ||
| 0.5 |
There was a problem hiding this comment.
again, would be better to have a good number here
|
Also there are a number of links to doc pages of other packages (scipy, sklearn) which do not work, it would be nice to fix them |
|
Should we consider that the new part of the doc on constraint generation fixes #135 ? |
|
For simplicity I'll merge this and make modifications myself in other PR |
This PR enhances the documentation by fixing issues about the doc and adding other improvements
Fixes #155
Fixes #149
Fixes #150
Fixes #135
TODO:
num_dimsremains (they should all be changed inton_components, I saw one in MLKR for instance)Ensure that the doctest work-> postponed, already opened in doctests not working #156Maybe add-> postponed, discussed in clean, uniformize the doc, fill in some blanks #227metric_learn.constraints.positive_negative_pairsdocstring