search

Sm00thix / IKPLS

Fast CPU and GPU Python implementations of Improved Kernel PLS by Dayal and MacGregor (1997) and Shortcutting Cross-Validation by Engstrøm (2024).
https://ikpls.readthedocs.io/en/latest/
Apache License 2.0
10 stars 4 forks source link

contributing doc - redeux & how to rebuild the doc #16

Closed parmentelat closed 6 months ago

parmentelat commented 6 months ago

I have had a second go at the contributing documentation, now that it is more accessible thanks to #10

turns out some of my initial concerns are still to be addressed

so I have restarted from scratch and put myself in a new potential contributor's shoes and came across a few further glitches

  1. the doc itself
    • the doc still does not address running the tests, which is one of the first things one would want to do
    • it does not either address rebuilding the doc itself
    • it could profitably mention the creation of a virtual env
  2. more importantly, I could not find a way to rebuild the doc locally (more on this below)

about 1. : to illustrate all this you can take a look at this page here: https://github.com/parmentelat/IKPLS/blob/contrib-doc/CONTRIBUTING.md

note: I was about to create a PR from that material, but refrained from doing so because I can neither read nor write nor preview reStructuredText, so I've had to move to markdown, which is maybe not what you'll want to do... regardless, the contents is what matters here anyways

about 2. : for rebuilding the doc, I had to mess with docs/Makefile like here https://github.com/parmentelat/IKPLS/commit/3f9a8329ff2393a670e6842e76cd71ee59aec654 as I can't access the builds page on readthedocs, I can't say whether the builds pass over there but it feels unlikely ?

parmentelat commented 6 months ago

as an aside, since in #15 you mention running benchmarks, it may be worth giving the recipe in this doc as well

parmentelat commented 6 months ago

and oops I forgot to link the meta-issue https://github.com/openjournals/joss-reviews/issues/6533

Sm00thix commented 6 months ago

Hi @parmentelat Thank you for the feedback, and thank you for the aid with the docs/Makefile. I will address each of your points here:

1.

the doc still does not address running the tests, which is one of the first things one would want to do

This was actually addressed in CONTRIBUTING.rst in step 4 under the headline "You want to make some kind of change to the code base". However, I admit that it was not very visible. Therefore, I have updated it to be more visible and put a more elaborate guideline under the headline "Run the test suite" in CONTRIBUTING.rst. Additionally, I updated the guidelines to use poetry which is also what the GitHub actions are using.

it does not either address rebuilding the doc itself

I have created a section named "Build the documentation" in CONTRIBUTING.rst to address this.

it could profitably mention the creation of a virtual env

Creating a virtual environment is not required for installation or usage of the ikpls package. Although, I agree using virtual environments is good practice, it really is up to a user whether or not they want to use them. Furthermore, setting up and managing virtual environments depend on the platform a user is working on - and there are many of these. There are resources available online for each of these platforms. As such, I believe a user is better off going to the primary source of information instead of me writing something that may be potentially be outdated in the future and which also has to be quite elaborate to accommodate all the possible platforms. With these points in mind, I urge you to reconsider this suggestion.

2.

about 2. : for rebuilding the doc, I had to mess with docs/Makefile like here https://github.com/parmentelat/IKPLS/commit/3f9a8329ff2393a670e6842e76cd71ee59aec654

Thank you for catching this. I have changed the docs/Makefile and docs/Make.bat to use "." as the source dir instead of docs/source.

as I can't access the builds page on readthedocs, I can't say whether the builds pass over there but it feels unlikely ?

On readthedocs, the build actually passed perfectly in spite of the docs/Makefile not working as intended locally. I guess readthedocs uses something else to build the docs on their end. However, as I mentioned above, I have updated the Makefile, and the build still passes on readthedocs. You should be able to verify it with this badge: https://readthedocs.org/projects/ikpls/badge/?version=latest.

as an aside, since in https://github.com/Sm00thix/IKPLS/issues/15 you mention running benchmarks, it may be worth giving the recipe in this doc as well

I have added a "Benchmarking" section to CONTRIBUTING.rst to address this. It simply points the reader to paper/README.md.

Sm00thix commented 6 months ago

Hi @parmentelat , I believe this issue is resolved with the changes mentioned in my previous comment in this thread and those we discussed over in #10 and #19 . If you agree, then we can close this issue.

parmentelat commented 6 months ago

agreed, thanks for this work, I appreciate the new layout, and believe the repo will be more readily usable for potential contributors