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

Restructure documentation and add or streamline beginner-friendly examples #265

Merged
merged 47 commits into from
May 25, 2022

Conversation

rhugonnet
Copy link
Contributor

@rhugonnet rhugonnet commented Apr 23, 2022

Final PR description

This PR includes:

  • A restructuration of documentation for clearer sections "Getting started", "Background", "Features", "Gallery of examples",
  • An update of the landing page,
  • The addition of beginner pages on "About xDEM", "How to install" and "Quick start",
  • A streamlining of existing examples to be more beginner-friendly,
  • A change in dev-environment.yml to pass tests dependant on GeoUtils without requiring a new release. See new issue Switch back from git-based to release-based GeoUtils in dev-environment #268 to switch back once our packages are mature enough.

New documentation can be viewed here: https://xdem-rhugonnet.readthedocs.io/en/doc_struc/

Resolves #258

We merge this PR early to match @adehecq's EGU talk.
Similar objectives described below will be addressed in later PRs.

Points of issue in original PR

  1. Structure of documentation (see Subsections of sphinx-gallery + Content headers for documentation #264)
  2. Location of test data VS Location of gallery examples (see Subsections of sphinx-gallery + Content headers for documentation #264 and Split doc into "Getting started" and "Advanced guide" #258)
  3. Structure of information in README (see Split and streamline information in README  #206)

@rhugonnet rhugonnet marked this pull request as draft April 23, 2022 18:51
Copy link
Contributor

@erikmannerfelt erikmannerfelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is a fantastic improvement! I only have some very small comments.

Sorry for the delay

docs/source/intro_robuststats.rst Outdated Show resolved Hide resolved
docs/source/intro_dems.rst Outdated Show resolved Hide resolved
docs/source/quick_start.rst Outdated Show resolved Hide resolved
docs/source/vertical_ref.rst Outdated Show resolved Hide resolved
examples/plot_icp_coregistration.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@rhugonnet rhugonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is perfect to have a more intuitive landing page + getting started examples for EGU 👍

We could merge this PR just for EGU, and advance further on the documentation (e.g. splitting of the examples) in a later one. Is that what you have in mind @adehecq ?

docs/source/index.rst Outdated Show resolved Hide resolved
docs/source/install_xdem.rst Outdated Show resolved Hide resolved
docs/source/quick_start.rst Outdated Show resolved Hide resolved
Sample data
-----------

*xdem* comes with some sample data that is used throughout this documentation to demonstrate the features. If not done already, the sample data can be downloaded with the command
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we talk about the package name we write "xDEM", and for the module sometimes xdem or xdem. Which one should we pick and stick to?
In the NumPy docs, looks like they stick to the package name "NumPy" without any kind of highlighting, and that the lower case, package-style naming numpy is only used with a submodule e.g. numpy.array

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I have no strict opinion on what we should use. In case of NumPy, the name is somewhat highlighted with the camel syntax. It feels a bit weird to write xdem in the text as if it was a regular word, so I like having a way to highlight it.
We agreed that lowercase is better than uppercase so that's why I went with the italic (xdem) and I think the code highlight (xdem) should be reserved for actual code so not in the text.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looked at what is done in the docs of Matplotlib, NumPy, SciPy, PyTorch, GPyTorch, I'd do the same: use the "normal text" package syntax that is camel-case/acronym-coded.
For us, it would correspond to using "xDEM" in normal text everywhere (in a way, its capitalization would serve to highlight the name).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still time to change this later: to avoid the discrepancy between the xdem and xdem in the docs of this PR, I changed into xDEM everywhere to mirror the syntax used in big packages as described above. It will be easier to replace if we want to, because with the capitalized letters it doesn't mix with the code ocurrences :)

docs/source/vertical_ref.rst Outdated Show resolved Hide resolved
docs/source/what_is_xdem.rst Outdated Show resolved Hide resolved
docs/source/what_is_xdem.rst Outdated Show resolved Hide resolved
@adehecq adehecq marked this pull request as ready for review May 25, 2022 12:53
@rhugonnet rhugonnet changed the title Restructure documentation, gallery examples and README Restructure documentation and add or streamline beginner-friendly examples May 25, 2022
@rhugonnet rhugonnet requested a review from adehecq May 25, 2022 20:08
Copy link
Member

@adehecq adehecq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for making the final push !!

@adehecq adehecq merged commit c809245 into GlacioHack:main May 25, 2022
@rhugonnet rhugonnet deleted the doc_struc branch September 9, 2022 15:05
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.

Split doc into "Getting started" and "Advanced guide"
3 participants