-
-
Notifications
You must be signed in to change notification settings - Fork 213
Big Rework of the Documentation #1432
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1432 +/- ##
========================================
Coverage 52.69% 52.69%
========================================
Files 36 36
Lines 4346 4346
========================================
Hits 2290 2290
Misses 2056 2056 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Please fix the regex in |
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.
I started reviewing but I wasn't entirely sure what to review.
Are there particular pointers, e.g., pages/tutorials that changed significantly? Things I really need to see before we merge? There are a number of things I noted that could be improved, e.g., using more common mkdocs-material features, such as admonitions and tabs. Those could also be iterated over in future PRs, which might be an easier way for me to give feedback as well (I could just set up a PR with my suggested improvements).
Other notes:
- The favicon is interestingly only applied to subpages only. I am guessing this is some caching artifact on my end, though even an in-cognito browser seems to exhibit this behavior.
| @@ -1,65 +1,79 @@ | |||
| # OpenML | |||
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.
We should just re-use the top-level README instead of introducing duplication, right? A docs/README.md is automatically shown on GitHub if there is no README.md in the root.
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.
Mostly but not fully, here we can have more details.
Maybe generally, be aware that I mostly took the state that already existed, which did not have a lot of native mkdocs structure and some sub-optimal code choices. I mostly refactored the content.
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.
here we can have more details
We can, but think it's fine not to in order to allow re-use and avoid desync.
docs/index.md
Outdated
|
|
||
| For more advanced installation information, please see the | ||
| ["Introduction"](../examples/20_basic/introduction_tutorial.py) example. | ||
| ["Introduction"](../examples/Basics/introduction_tutorial.py) example. |
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.
| ["Introduction"](../examples/Basics/introduction_tutorial.py) example. | |
| ["Introduction"](../examples/Basics/introduction_tutorial) example. |
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.
There are a number of warnings when serving the docs, including this broken link.
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.
When I build locally, it also throws warnings for non-broken links, so I do not know how to judge which one is correct
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.
I think this link is marked as broken, as it does not exist at the time the link checker comes in. The notebooks are only built later. It works locally
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.
It does for you? I tried this one locally and it did not work for me.
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.
Yes (with the committed fix from you)
| @@ -0,0 +1,22 @@ | |||
| # %% [markdown] | |||
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 there a reason this is a Python file instead of markdown?
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.
It did not show up on the docs as .md.
Looking into it, it is because with markdown this file is not copied into the docs folder from the pre-existing script, I think. (scripts/gen_ref_pages.py)
|
I guess you can review all the changes and documentation as much as you like as it is generally very outdaed and I mostly moved it or deleted stuff |
Co-authored-by: Pieter Gijsbers <p.gijsbers@tue.nl>
Co-authored-by: Pieter Gijsbers <p.gijsbers@tue.nl>
|
Fun fact: the regrex test crashing only happened because of PyCharms refactor logic also changing one but not the other value. Without it, I would not have even known that the link to the documentation in the code is totally wrong already. |
|
Alright, I fixed some minor things and the rest looks good. I still need some more clarification on these : @LennartPurucker In Advanced/create_upload_tutorial.py, the title is “Dataset is a numpy array” instead of Creating and uploading datasets. I am unable to figure out how to change this :/ /Advanced/tasks_tutorial/ : resampling link leads to an empty search Details : this doesnt seem like a good title :/ details of what exactly? - the page has information about config and docker and stuff Openml.org/cite doesnt exist in contributing |
|
|
Okay cool, Ill do that |
|
Okay its good to go from my side |
|
Did you update the citation link? |
|
Yep! |
|
Supi, LGTM from me, feel free to merge! |
SubhadityaMukherjee
left a comment
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.
Approving this, but it remains to be seen if the building works now. Can't really test it any other way
Metadata
Details
See changes.