Skip to content

Conversation

@castedo
Copy link
Member

@castedo castedo commented Oct 26, 2020

For #844. This builds on top of #920 which did not constify all the "stats" functions in tree.h.

This PR constifies all those stats functions and fixes duplicated typedefs by keeping them defined only once in trees.h.

@AdminBot-tskit
Copy link
Collaborator

📖 Docs for this PR can be previewed here

@codecov
Copy link

codecov bot commented Oct 27, 2020

Codecov Report

Merging #939 into main will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #939   +/-   ##
=======================================
  Coverage   93.40%   93.40%           
=======================================
  Files          25       25           
  Lines       20335    20335           
  Branches      824      824           
=======================================
  Hits        18994    18994           
  Misses       1304     1304           
  Partials       37       37           
Impacted Files Coverage Δ
c/tskit/trees.c 94.75% <100.00%> (ø)
_tskitmodule.c 90.21% <0.00%> (ø)

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 d373b79...fdcabbf. Read the comment docs.

@jeromekelleher
Copy link
Member

Thanks @castedo - would you mind marking these as "draft" PRs until they're ready for review/merging? It's tricky for me to keep track of what's what and know what GitHub updates I should pay attention to.

@castedo castedo marked this pull request as draft October 27, 2020 15:40
@castedo castedo force-pushed the pr/const_c_stats_api branch from c63e796 to 2709305 Compare October 28, 2020 20:56
@castedo
Copy link
Member Author

castedo commented Oct 28, 2020

Roughly speaking, this PR just constifies all the stats functions in trees.h/c.

Because this PR still does not change the types in trees.h, some stats functions can not yet be constified (e.g. tsk_treeseq_kc_distance). That happens in the next and final PR #940 .

Two functions have const params that are fun, e.g.

int tsk_treeseq_mean_descendants(..., tsk_id_t *const *reference_sets, ... )

This is a fun case where "Mediterranean/romance" noun-adjective order is the only way to express what stays constant. It might be intuitive reading it "in Spanish", e.g. "tsk_id_t puntero constante puntero" where the const is an adjective to the noun to the left, not to the right.

Heads up there's a non-stats tweak:

int tsk_treeseq_init(tsk_treeseq_t *self, const tsk_table_collection_t *tables, ... )

because the tables are copied.

@castedo castedo marked this pull request as ready for review October 28, 2020 21:04
Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

This is excellent @castedo, a major improvement in documenting this rather tricky API. Some minor comments and questions.

@castedo castedo force-pushed the pr/const_c_stats_api branch from 2709305 to 5052b05 Compare October 29, 2020 13:40
@castedo
Copy link
Member Author

castedo commented Nov 2, 2020

LGTM for merging.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

Let's merge!

@mergify mergify bot merged commit a004019 into tskit-dev:main Nov 2, 2020
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.

4 participants