Skip to content

Conversation

@benjeffery
Copy link
Member

Description

Adds fleshed out examples to the example module and some docs to the README. Not finished yet as I assume we want doc strings on everything?

Fixes #864

PR Checklist:

  • Tests that fully cover new/changed functionality.
  • Documentation including tutorial content if appropriate.
  • Changelogs, if there are API changes.

@benjeffery
Copy link
Member Author

@jeromekelleher Still draft as I assume we want docstrings - but thought it worth you having a look at the C Python code I added to the examples as I likely missed a detail or more standard way of doing something!

@AdminBot-tskit
Copy link
Collaborator

📖 Docs for this PR can be previewed here

@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #1004 (cf31f4f) into main (471ad9e) will decrease coverage by 0.04%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1004      +/-   ##
==========================================
- Coverage   93.72%   93.68%   -0.05%     
==========================================
  Files          26       26              
  Lines       20937    20985      +48     
  Branches      875      875              
==========================================
+ Hits        19623    19659      +36     
- Misses       1277     1289      +12     
  Partials       37       37              
Flag Coverage Δ
c-tests 92.49% <ø> (ø)
lwt-tests 92.80% <75.00%> (-0.78%) ⬇️
python-c-tests 94.82% <75.00%> (-0.09%) ⬇️
python-tests 98.61% <ø> (ø)

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

Impacted Files Coverage Δ
python/lwt_interface/example_c_module.c 75.47% <75.00%> (-4.53%) ⬇️

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 471ad9e...cf31f4f. Read the comment docs.

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.

Looks great. What would be really super-useful here would be an example of how to use the lwt interface with Cython - but maybe we don't want to get into this.

@benjeffery benjeffery force-pushed the lwt-docs branch 3 times, most recently from 2a09976 to 220e105 Compare November 19, 2020 16:03
@benjeffery
Copy link
Member Author

@jeromekelleher I've made some changes and fleshed out the example to actually build a tree sequence. I've also removed the creating of a table collection on the C side as we should discourage this.

@benjeffery benjeffery marked this pull request as ready for review November 19, 2020 16:23
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.

LGTM, some minor nits

@benjeffery
Copy link
Member Author

@jeromekelleher fixed up.

@AdminBot-tskit
Copy link
Collaborator

📖 Docs for this PR can be previewed here

@benjeffery
Copy link
Member Author

Docs for this PR can be previewed here

Ah-ha the bot thinks there are doc changes as it is comparing to main which has commits that this is PR not rebased on to. Hmm. Not worth worrying about, but would be possible for it to compare to the base commit of the PR.

@benjeffery
Copy link
Member Author

@terhorst We've done some work on the Python C extension interface examples and docs here - would you mind seeing if it makes sense? We'll be doing a cython example too eventually.

@mergify mergify bot merged commit accfac6 into tskit-dev:main Nov 26, 2020
@terhorst
Copy link
Contributor

@terhorst We've done some work on the Python C extension interface examples and docs here - would you mind seeing if it makes sense? We'll be doing a cython example too eventually.

Hi @benjeffery, sorry, I mentally filed this thread away to respond ... and then promptly threw the filing cabinet out the window ;) In short, I think this is great! Super helpful for what I wanted to accomplish. The only missing piece of the puzzle for me was Cython integration. Cython automatically generates PyMODINIT_FUNC and, as far as I can tell, there is no (supported) way to inject a call to register_lwt_class(module) in there. (I say supported because I was able to get it to work by externing the module variable and dropping the call directly into the top-level module code, but this felt very hacky and liable to break at some point in the future.) Also there are some issues with void pointers if you try to use tskit_lwt_interface.h in C++ code, but it's not a showstopper.

What I ended up doing was creating an extremely minimal C module, say _lwtc.c, that essentially just calls the register function. Then in Cython I wrapped it with an extension type declaration, i.e.

    ctypedef class xsmc._lwtc.LightweightTableCollection [object LightweightTableCollection]:
        cdef tsk_table_collection_t *tables

The key is being able to access the table collection pointer from Cython code. Once that's done, you can call all the rest of the tsk_ API and you're off to the races.

@jeromekelleher
Copy link
Member

Awesome, thanks for the update @terhorst! Do you have a link to the code please? We'd like to make a minimal example so that others can benefit from your work here!

@terhorst
Copy link
Contributor

Sure, a minimal example is to define the LightweightTreeSequence module via

https://github.com/terhorst/xsmc/blob/arg/src/_lwtc.c

(essentially your example code with all the nonessentials stripped out), and then call it as so:

https://github.com/terhorst/xsmc/blob/arg/xsmc/_viterbi.pyx#L154-L157

Note that you'll need to have defined all the various tskit headers; that happens here:

https://github.com/terhorst/xsmc/blob/arg/xsmc/_tskit.pxd.

@benjeffery
Copy link
Member Author

benjeffery commented Jan 20, 2021

@terhorst Great stuff, is there any chance you have the time to polish the code you've linked to into a minimal example in the tskit codebase? No worries if not!

@benjeffery benjeffery deleted the lwt-docs branch January 20, 2021 00:39
@terhorst
Copy link
Contributor

Here is a Cythonized version of one of the C API examples from the docs:

https://github.com/terhorst/tskit_cython_example

@jeromekelleher
Copy link
Member

This looks great, thanks @terhorst! @benjeffery, what's best way of linking this into our docs? Would it be best to get a copy into the tskit repo so we can make sure we don't break it (like the current C example?)

@jeromekelleher
Copy link
Member

Moving the discussion to #1124 so we don't lose track of it.

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.

Document LightweightTableCollection interface and usage

4 participants