Add order argument to ts.nodes()

[hyanwong]

Fixes #2370

This uses a lambda function, which make it quite a neat and flexible change without affecting performance of the SimpleContainerSequence.__getitem__ function. Is that OK?

It also means we can do things like:

ts = tskit.Tree.generate_balanced(10).tree_sequence
nodes_in_time_order = ts.nodes(order="timedesc")
nodes_in_time_order[0]  # returns node 18

But I think that's quite reasonable behaviour. If this seems like the right approach, I'll add some tests.

[codecov]

Codecov Report

Merging #2471 (42fbc5f) into main (70de29f) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 42fbc5f differs from pull request most recent head c4684cd. Consider uploading reports for the commit c4684cd to get more accurate results

@@            Coverage Diff             @@
##             main    #2471      +/-   ##
==========================================
- Coverage   93.43%   93.40%   -0.03%     
==========================================
  Files          28       28              
  Lines       27401    27321      -80     
  Branches     1255     1246       -9     
==========================================
- Hits        25601    25520      -81     
  Misses       1766     1766              
- Partials       34       35       +1     

Flag	Coverage Δ
c-tests	92.24% <0.00%> (ø)
lwt-tests	89.05% <0.00%> (ø)
python-c-tests	71.17% <0.00%> (-0.01%)	⬇️
python-tests	98.94% <0.00%> (-0.02%)	⬇️

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

Impacted Files	Coverage Δ
python/tskit/trees.py	98.64% <0.00%> (-0.09%)	⬇️
python/tskit/tables.py	98.92% <0.00%> (-0.04%)	⬇️
python/tskit/combinatorics.py	99.36% <0.00%> (-0.01%)	⬇️

---

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 70de29f...c4684cd. Read the comment docs.

[jeromekelleher]

LGTM. Not obvious to me why timedesc would have ascending node ID as the secondary key though (or at least I can't think of any practical usage of this). Wouldn't it be simpler if timedesc was equal to reversed timeasc?

[hyanwong]

Wouldn't it be simpler if timedesc was equal to reversed timeasc

There's no need to have a "timedesc" in that case, as the iterator is reversible: reversed(ts.nodes(order="timeasc")). I put "timedesc" in there in case the alternative was needed, for instance traversing down through an ARG while retaining the "standard" order for ties. But it's a very niche case. Maybe we remove "timedesc" until such time as someone else wants that particular functionality.

[jeromekelleher]

Let's remove timedesc then, so we're not painting ourselves into a corner with the semantics. Maybe document the trick about for reversing?

[hyanwong]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[jeromekelleher]

LGTM, some comments on testing.

[benjeffery]

@hyanwong Shall I review now?

[hyanwong]

@hyanwong Shall I review now?

Yes please.