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

[WIP] improvement suggestions for the label guide documentation (until sorting dimensions chapter) #1699

Conversation

Eva-Lotte
Copy link
Contributor

Description

@codecov
Copy link

codecov bot commented May 20, 2021

Codecov Report

Merging #1699 (01fadad) into main (1a39374) will not change coverage.
The diff coverage is n/a.

❗ Current head 01fadad differs from pull request most recent head 549b643. Consider uploading reports for the commit 549b643 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1699   +/-   ##
=======================================
  Coverage   90.86%   90.86%           
=======================================
  Files         108      108           
  Lines       11818    11818           
=======================================
  Hits        10738    10738           
  Misses       1080     1080           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a39374...549b643. Read the comment docs.

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

Thanks ❤️ these really improve the clarity of the guide.

Provided we know that the coordinate values shown for theta correspond to the `school` dimension,
we can plot only ``tau`` to better inspect it's 1.03 :func:`~arviz.rhat` and
``theta`` for ``Choate`` and ``St. Paul's``, the ones with higher means:
ArviZ supports label based indexing.
Copy link
Member

@OriolAbril OriolAbril May 20, 2021

Choose a reason for hiding this comment

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

I would like to still be explicit about xarray even though I'm not sure how.

Many users only use ArviZ functions, plots and diagnostics and are not aware that all of that is powered by xarray under the hood. This is particularly relevant for researchers who then don't cite xarray when publishing even though they did cite ArviZ. It is not "our" problem per se, but I think we should try to nudge users towards the right citations and it is also bad for us indirectly because xarray is generally undercited which dificults it getting grants and slows down its development.

Comment on lines 29 to 31
For a case where the coordinate values shown for the theta variable correspond to the `school` dimension
you can plot only the ``tau`` variable to better inspect its 1.03 :func:`~arviz.rhat` value,
only the ``theta`` variable to the ``Choate`` and ``St. Paul's`` coordinates, which have higher means, you can ploy only the ``theta`` variable by running the following command:
Copy link
Member

Choose a reason for hiding this comment

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

My original description wasn't very good, but it tried to say that we are plotting all the things that were described, we are not plotting only the theta variable. How about something like:

Given that summary by default shows the coordinate values and not their indices or some aliases, 
you can use what you see to subset the data. 
The summary shows that ``tau`` has 1.03 :func:`~arviz.rhat` value which deserves closer inspection. 
You can indicate ArviZ to plot ``tau`` by including it in the ``var_names`` argument. 
Moreover, you may also want to inspect closely the ``theta`` values for the ``Choate`` 
and ``St. Paul's`` coordinates because of their higher means. 
You can do so by including ``theta`` too in ``var_names`` and 
using the ``coords`` argument to select only these two coordinate values for the ``school`` dimension. 
You can generate this plot with the following command:

doc/source/user_guide/label_guide.rst Outdated Show resolved Hide resolved

Fear not, we can use the labeller argument to customize the labels.
The ``arviz.labels`` module contains some classes that cover some common customization classes.
To create a report on Deerfield, Hotchkiss and Lawrenceville schools for the probability of ``theta > 5``, use the labeller argument to customize labels.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To create a report on Deerfield, Hotchkiss and Lawrenceville schools for the probability of ``theta > 5``, use the labeller argument to customize labels.
You'll now create a report on Deerfield, Hotchkiss and Lawrenceville schools for the probability of ``theta > 5`` and use the labeller argument to customize labels.

~~~~~~~~~~~~~~~~~~~~

- For a list of labellers available in ArviZ, see the :ref:`the API reference page <labeller_api>`.
- For common customization classes, see the ``arviz.labels`` module.
Copy link
Member

Choose a reason for hiding this comment

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

The api reference has the docs for the arviz.labels module, the original last line tried to give a choice to readers. At this point they have seen how to use the labeller argument. Therefore, if they only want to do some more or less simple changes to the labels, they don't need to continue reading this guide, but they will probably need to read the api reference to find the class that suits their needs (which may be IdxLabeller instead of MapLabeller). Does this make sense? Maybe deleting this 2nd bullet point is enough already?

Also, how about using the seealso directive? https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-seealso

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the hint with the seealso directive! This is what I was looking for.

doc/source/user_guide/label_guide.rst Outdated Show resolved Hide resolved
doc/source/user_guide/label_guide.rst Outdated Show resolved Hide resolved
@@ -97,22 +104,25 @@ Sorting variable names
Sorting coordinate values
.........................

We may also want to sort the schools by their mean.
To do so we first have to get the means of each school:
To sort coordinate values by mean you have to locate the means of each school and then use the DataArray result to sort the coordinate values.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To sort coordinate values by mean you have to locate the means of each school and then use the DataArray result to sort the coordinate values.
To sort coordinate values you have to first define the order and store it, then use that result to sort the coordinate values.

doc/source/user_guide/label_guide.rst Outdated Show resolved Hide resolved
doc/source/user_guide/label_guide.rst Outdated Show resolved Hide resolved
@Eva-Lotte
Copy link
Contributor Author

Thank you very much for your review!


Example: Using the labeller argument
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

To create a report on Deerfield, Hotchkiss and Lawrenceville schools for the probability of ``theta > 5``, use the labeller argument to customize labels.
To create a report on Deerfield, Hotchkiss and Lawrenceville schools for the probability of ``theta > 5`` and use the labeller argument to customize labels.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence now feels incomplete. It starts with "to create" and a description on what to create but nothing on how to create it.

A bit more context in case it helps. Here there are two goals: creating a report of theta > 5 and using the labeller to customize the labels.

One can be done without the other, but instead of showing how to use the labeller (which I think should be done in the example in he docstring that is still wip), I wanted to show how to use the labeller to solve a more specific and real task. When exploring the mode ourselves, we won't generally care much about the labels, having the same thing as the code is fine. However, if generating a report to be published or presented, we'll probably want to take better care of the presentation, and match the labels to the labels in the equations of the paper instead of variables in the code. Hence the MapLabeller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for providing a bit of context to this matter. The way I see it, since this piece of text is inside of the example, we don't even need the first part of the sentence.

doc/source/user_guide/label_guide.rst Outdated Show resolved Hide resolved
@@ -97,22 +103,25 @@ Sorting variable names
Sorting coordinate values
.........................

We may also want to sort the schools by their mean.
To do so we first have to get the means of each school:
To sort coordinate values you have to define the order, store it, and use the result to sort the coordinate values.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To sort coordinate values you have to define the order, store it, and use the result to sort the coordinate values.
To sort coordinate values you have to define the order, store it, and use the result to sort the coordinate values.
The order can be defined by performing some operations on our xarray objects (like it is shown in the example below)
or by manually creating a list with the desired order.

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

Looks great, only thing missing is adding to the changelog and merging.

@Eva-Lotte
Copy link
Contributor Author

Thank you, Oriol! I added the changes to the changelog! I can't seem to merge the PR, probably because of access rights?

@OriolAbril OriolAbril merged commit 1997afe into arviz-devs:main May 27, 2021
@OriolAbril
Copy link
Member

Yeah, only core contributors have merge privilieges, I wasn't very clear in my previous post. Thanks for the PR!

mjhajharia pushed a commit to mjhajharia/arviz that referenced this pull request Jun 7, 2021
…tation (arviz-devs#1699)

* documentation improvement suggestions for the label guide

* Addressed feedback

* Update doc/source/user_guide/label_guide.rst

Co-authored-by: Oriol Abril-Pla <[email protected]>

* feedback addressed

* added changes to CHANGELOG.md

Co-authored-by: Oriol Abril-Pla <[email protected]>
mjhajharia pushed a commit to mjhajharia/arviz that referenced this pull request Jun 7, 2021
…tation (arviz-devs#1699)

* documentation improvement suggestions for the label guide

* Addressed feedback

* Update doc/source/user_guide/label_guide.rst

Co-authored-by: Oriol Abril-Pla <[email protected]>

* feedback addressed

* added changes to CHANGELOG.md

Co-authored-by: Oriol Abril-Pla <[email protected]>
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.

2 participants