Skip to content
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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

mvargas33
Copy link
Contributor

@mvargas33 mvargas33 commented Oct 21, 2021

@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:

  • Contributing: Introduction, values, general guideline, how to contribute code (PR process), how to test, how to compile the docs.
  • Metric learn governance: Roles, decision-making process, in general. How to proceed with API changes (MLEP)
  • Implement a new algorithm: Criteria of selection, and how to proceed.
  • API Structure: A quick review of the API for devs to know what classes to inherit from, and wich methods to implement, and where.
  • MLEP Template: The template to be used in Github discussion for major changes.

What is left:

  • How to make a release
  • How to publish the docs (the gh-page branch thing)
  • How to update at Pypi and Conda.

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

Copy link
Member

@terrytangyuan terrytangyuan left a 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.

@mvargas33
Copy link
Contributor Author

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! 🧙‍♂️
Max

Ps: I suggest to merge #338 first to remove all warnings when building this PR.

@bellet bellet linked an issue Nov 4, 2021 that may be closed by this pull request
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
Copy link
Contributor

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.
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] Add developer documentation page
3 participants