scikit-learn-contrib / scikit-matter

A collection of scikit-learn compatible utilities that implement methods born out of the materials science and chemistry communities
https://scikit-matter.readthedocs.io/en/v0.2.0/
BSD 3-Clause "New" or "Revised" License
70 stars 18 forks source link

Modernize package infrastructure #172

Closed PicoCentauri closed 1 year ago

PicoCentauri commented 1 year ago

This PR updates a number of the infrastructure of the package:

  1. Going from a tox multi-line string to an independent tox.ini file. The latter one is easier to maintain as a huge multi-line string and also consistent with the other projects.
  2. Adding tox sections for linting, building the documentation and formatting the code
  3. Move skmatter to src/skmatter
  4. Rename docs/source -> docs/src (same is in other lab-cosmo projects)
  5. Translate from setup.py, setup.cfg into a single pyproject.toml file
  6. Add a documentation link action
  7. Add Python 3.11 to the CI test matrix
  8. Consistent use of Python 3.10 for docs, lint and ReadTheDocs in the CI
  9. Add tests for windows and macos
  10. Update all gh-action dependencies to the lates version
  11. Update contributors.txt
  12. Update Rose's mail address
  13. Use isort and flake8 settings of the other lab-cosmo projects
  14. Following 7 use 88 characters for all files including docs and comments
  15. Remove numpy as an explict dependency. The whole package depends on scikit-learn which depends on numpy. I think we do not need to list numpy again in the dependencies if we stick with the same version as they do (which we should).
PicoCentauri commented 1 year ago

Thanks for your review @Luthaf! I implemented the useful suggestions. If you are happy I would also like to have @rosecers opinion before we apply these changes.

PicoCentauri commented 1 year ago

Going through, all looks generally fine, but I'm worried that the new linting doesn't look as nice. Can we set it up with the same rules as before?

Thanks. Black is exactly the same. The only thing which is slightly changed is how isort works. Packages are sorted according to: FUTURE, STDLIB, THIRDPARTY, FIRSTPARTY, LOCALFOLDER, which is the default. I think this order is much easier to read; especially the splitting between THIRDPARTY (numpy, sklearn) and FIRSTPARTY (skmatter).

rosecers commented 1 year ago

So I get the 88 char/line thing from a code-formatting standpoint, but as a code reader, I find it very frustrating. Industry-standard is 80 -- any arguments for/against doing that instead?

PicoCentauri commented 1 year ago

I would stick with 88 characters. 88 chars/line is the current way of skmatter since we use black as the formatter. It was only not enabled for the docstring and not applied (and still is not) for the documentation pages. We all have widescreen monitors. Additional eight characters per line lead to fewer line breaks and more readable code.

Also, I am not sure if 80 is still the industry standard. sklearn uses 88 and even the greatLinus Torvald is saying that the time of 80 chars/line are over.

ceriottm commented 1 year ago

Ah, the good ole' Linus' rant. FWIW I'm all for longer lines.

On Wed, 29 Mar 2023 at 19:57, Philip Loche @.***> wrote:

I would stick with 88 characters. 88 chars/line is the current way of skmatter since we use black as the formatter. It was only not enabled for the docstring and not applied (and still is not) for the documentation pages. We all have widescreen monitors. Additional eight characters per line lead to fewer line breaks and more readable code.

Also, I am not sure if 80 is still the industry standard. sklearn uses 88 https://github.com/scikit-learn/scikit-learn/blob/70c489f1273a5ff877e61750b3a69590bc002b6f/pyproject.toml#L23 and even the greatLinus Torvald is saying that the time of 80 chars/line are over https://lkml.org/lkml/2020/5/29/1038.

— Reply to this email directly, view it on GitHub https://github.com/lab-cosmo/scikit-matter/pull/172#issuecomment-1489055393, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIREZ6FOQPWAEV675PXLJ3W6RZYVANCNFSM6AAAAAAWI5AISM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

rosecers commented 1 year ago

I’d advocate for even longer, ~100char!

On 29 Mar 2023, at 14:29, Michele Ceriotti @.***> wrote:

Ah, the good ole' Linus' rant. FWIW I'm all for longer lines.

On Wed, 29 Mar 2023 at 19:57, Philip Loche @.***> wrote:

I would stick with 88 characters. 88 chars/line is the current way of skmatter since we use black as the formatter. It was only not enabled for the docstring and not applied (and still is not) for the documentation pages. We all have widescreen monitors. Additional eight characters per line lead to fewer line breaks and more readable code.

Also, I am not sure if 80 is still the industry standard. sklearn uses 88 https://github.com/scikit-learn/scikit-learn/blob/70c489f1273a5ff877e61750b3a69590bc002b6f/pyproject.toml#L23 and even the greatLinus Torvald is saying that the time of 80 chars/line are over https://lkml.org/lkml/2020/5/29/1038.

— Reply to this email directly, view it on GitHub https://github.com/lab-cosmo/scikit-matter/pull/172#issuecomment-1489055393, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIREZ6FOQPWAEV675PXLJ3W6RZYVANCNFSM6AAAAAAWI5AISM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/lab-cosmo/scikit-matter/pull/172#issuecomment-1489182638, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALKVP3WN5MNYNMAY2K2XOWDW6SEQFANCNFSM6AAAAAAWI5AISM. You are receiving this because you were mentioned.

PicoCentauri commented 1 year ago

Okay, since this might be a longer and intensive discussion I suggest we keep the 88 chars/line in within this PR and open an issue about how we handle this in the future. One reason is that I would like to touch more files of the repo but this PR is already huge and complex.