Changes in documentation. Rephrasing, fixed examples, standarized notation, etc. #274
Conversation
doc/index.rst
Outdated
| @@ -52,7 +52,7 @@ Documentation outline | |||
|
|
|||
| auto_examples/index | |||
|
|
|||
| :ref:`genindex` | :ref:`modindex` | :ref:`search` | |||
| :ref:`genindex` | :doc:`Modules <./metric_learn>` | :ref:`search` | |||
There was a problem hiding this comment.
What does this do?
There was a problem hiding this comment.
For some reason, sphinx wasn't creating the modules index so it raised a 404 error. As a quick fix, I replaced it by the modules list (http://contrib.scikit-learn.org/metric-learn/metric_learn.html).
There was a problem hiding this comment.
This page is already linked a bit above on the index page, so perhaps we could simply remove it?
Or, fix the problem preventing the generation of this file
There was a problem hiding this comment.
I am not knowledgeable in sphinx so I can't find a quick fix, in the meantime, I will remove it.
metric_learn/rca.py
Outdated
| >>> from metric_learn import RCA | ||
| >>> pairs = [[[1.2, 7.5], [1.3, 1.5]], | ||
| >>> [[6.4, 2.6], [6.2, 9.7]], | ||
| >>> [[1.3, 4.5], [3.2, 4.6]], | ||
| >>> [[6.2, 5.5], [5.4, 5.4]]] | ||
| >>> y = [1, 1, -1, -1] | ||
| >>> # in this task we want points where the first feature is close to be | ||
| >>> # closer to each other, no matter how close the second feature is | ||
| >>> rca = RCA() | ||
| >>> rca.fit(pairs, y) |
There was a problem hiding this comment.
This example is wrong: RCA learns from chunks, not from pairs
There was a problem hiding this comment.
I took this example from the doc/weakly_supervised.rst RCA section. I will fix them both then.
There was a problem hiding this comment.
Then it would also be necessary to make a new section in Weakly Supervised Metric Learning called "Learning on chunks"? Right now RCA is under "Learning on pairs".
There was a problem hiding this comment.
Yes, there is an issue on that, see #237 and comments therein
Since the idea of chunks is very specific to RCA, with a small abuse of language we were considering listing it under the supervised algorithms, even though the supervision is weaker
There was a problem hiding this comment.
I think that it should be in the weak learners section for consistency's sake, as there is a *_supervised version, even though the supervision is not so weak. Would it be worth it to make a section just for RCA learning on chunks?
There was a problem hiding this comment.
Sure, why not, although it is unlikely to apply to another algorithm other than RCA in the future
If you are willing to do this, please go ahead. Probably a separate PR would be best for clarity
There was a problem hiding this comment.
Alright then, in the meantime I fixed the RCA examples as they are.
|
LGTM! Thanks for the changes and congrats for your first contribution @grudloff :-) |
No description provided.