-
Notifications
You must be signed in to change notification settings - Fork 234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DOC] [WIP] Developers documentation page #340
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see the formal process documented. Let me know when this is ready for review.
Hello! The branch is ready for a review. I added a last section called "Publishing Releases" that has all the information explained at #250 and #259. I emphasize that this documentation should be treated as a draft, as many parts of the contributing process is not yet defined. This PR is an opportunity to clarify it and reach consensus in How to collaborate. It would be great if @perimosocordiae, @wdevazelhes and @bellet could also review this to have a broad opinion about it. Best! 🧙♂️ Ps: I suggest to merge #338 first to remove all warnings when building this PR. |
To avoid duplicating work, it is highly advised that you search | ||
through the issue tracker and the PR list. If in doubt about duplicated | ||
work, or if you want to work on a non-trivial feature, it’s recommended | ||
to first open an issue in the issue tracker to get some feedbacks from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nitpick: feedbacks -> feedback
One easy way to find an issue to work on is by applying the “help wanted” | ||
label in your search. This lists all the issues that have been unclaimed | ||
so far. In order to claim an issue for yourself, please comment exactly | ||
`take` on it for the CI to automatically assign the issue to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually work? I've never seen this before.
Building the docs | ||
^^^^^^^^^^^^^^^^^ | ||
|
||
To build the docs is always recommended to start with a fresh virtual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is -> it is
|
||
1. Make sure that all versions numbers are updated in ``metric_learn/_version.py``` | ||
and in ``doc/conf.py```. | ||
2. Make sure that the final year date in doc/conf.py after copytight is the right |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: copyright
Estimators in metric-learn all have a ``preprocessor`` option at instantiation. | ||
Filling this argument allows them to take more compact input representation | ||
when fitting, predicting etc... | ||
.. rst-class:: deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still deprecating this? It feels a bit out of place for this PR.
@bellet @perimosocordiae @terrytangyuan @wdevazelhes
Motivated by #259 , I made this docs for new developers like a guide in How to contribute to the package. I followed the scikit-learn guideline here, but talking with @bellet it's better to keep things simple in terms of governance, for instance.
I also considered comments at #205 and in #13 .
I also propose that for API or major changes, a MLEP (Metric Learning Enhancement Proposal) document is needed, being Github Discussions the palce to put it and review it, because sometimes, huge API changes are linked to more than one PR. Take the OASIS discussion at #336 as a very simple and informal MLEP. (In general I took this idea from sklearn).
The main sections are:
What is left:
And because this has not been discussed in the past, take this draft as what it is: a draft. Moreover the governance part, and the MLEP part. Maybe something much simpler is enough, like the Github discussion #336 that I did, but with a general template.
Best! 😁
Ps: I'm testing CSS usage in some parts, ignore it