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

Unifying stat method desc and linking to math summary #3005

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gregorgorjanc
Copy link
Member

I was working through the stats tutorial at https://tskit.dev/tutorials/analysing_tree_sequences.html (incoming PR) and thought that linking stats/math summaries (https://tskit.dev/tskit/docs/stable/stats.html#summary-functions) to method docs would be very useful.

I also followed a style from the diversity method doc (what is the stat, then links to sample_sets, windows, ...).

@petrelharp can you have a look at this PR?

Copy link

codecov bot commented Sep 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.82%. Comparing base (bc5a73c) to head (6371edc).
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3005      +/-   ##
==========================================
+ Coverage   89.81%   89.82%   +0.01%     
==========================================
  Files          29       29              
  Lines       31979    31995      +16     
  Branches     6190     6195       +5     
==========================================
+ Hits        28721    28739      +18     
+ Misses       1861     1859       -2     
  Partials     1397     1397              
Flag Coverage Δ
c-tests 86.69% <ø> (ø)
lwt-tests 80.78% <ø> (ø)
python-c-tests 89.05% <ø> (ø)
python-tests 99.04% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
python/tskit/trees.py 98.80% <100.00%> (ø)

... and 1 file with indirect coverage changes

@@ -8246,6 +8249,9 @@ def genetic_relatedness(
"""
Computes genetic relatedness between (and within) pairs of
sets of nodes from ``sample_sets``.
Please see the :ref:`summary functions <sec_stats_summary_functions>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Please see the :ref:`summary functions <sec_stats_summary_functions>`
See the :ref:`summary functions <sec_stats_summary_functions>`

For consistency.

Comment on lines 8488 to 8489
Please see the :ref:`summary functions <sec_stats_summary_functions>`
section on the exact definition of the calculated statistic.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't actually think the "summary function" is a good place to refer people to here. That is a definition, but it only makes sense if someone understands How It All Works. The approach we've taken in other statistics is to provide a more plain (but equivalent) definition. I think here the thing to do is to just refer to the docs for genetic_relatedness, because this function's notion of genetic relatedness is the same as that one.

@petrelharp
Copy link
Contributor

In general I like the rearrangments, but I don't think we should add "please see summary functions for a precise definition". Instead, we should make what's said there in the documentation precise enough, if it isn't already? I am pretty sure that what's there for diversity, for instance, is precise - did you find it ambiguous? The 'summary function' definition is nice and concrete once you understand it, but most readers won't be on board with this. Some other way of referring to the summary functions could be helpful but more like "if you want the computational details, see XXX"?

@gregorgorjanc
Copy link
Member Author

@petrelharp I followed your suggestions in the last commit.

I thought adding the link to computational/mathematical definition would be useful to provide concrete/mathematical detail of what is calculated. This makes things concrete, but since it's only a link, it does not overwhelm a user (I agree that simple descriptive text should be preferred). Also, you have written these math details for some stats, so adding these links exposes/leverages that part of the doc (most users will only look at function docs and won't go over the whole stats doc to find math detail).

Happy to iterate.

@benjeffery benjeffery added this to the Python 0.5.9 milestone Oct 7, 2024
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.

3 participants