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

add keep_unary for pedigrees and dtwf #2145

Closed

Conversation

GertjanBisschop
Copy link
Member

@GertjanBisschop GertjanBisschop commented Dec 9, 2022

This pull request stores all nodes through which some ancestral material passes for pedigrees and dtwf simulations when the flag record_unary=True is set. This is a first pass to allow for testing (see #2132).
This pull request requires defining a MSP_NODE_IS_PASS_THROUGH_EVENT (see here). Is this as simple as simply using the next available number in line (#define MSP_NODE_IS_PT_EVENT (1u << 22))?
As with #2130 unit tests are still basic and require further work.

@jeromekelleher
Copy link
Member

Nice!

@sgravel I think this should more-or-less do what we want with the pedigree sims, so we could start thinking about trying it out and seeing what we get?

@sgravel
Copy link

sgravel commented Dec 9, 2022

Perfect! Presumably this does not record the ancestors above the MRCA yet, right?

@GertjanBisschop
Copy link
Member Author

Perfect! Presumably this does not record the ancestors above the MRCA yet, right?

Yes, that's correct @sgravel. This would require an additional flag indicating the stopping point of the simulation. We have been discussing this exact issue though while thinking about potential performance improvements (see #2121).

@sgravel
Copy link

sgravel commented Dec 9, 2022

Ok, we'll do some testing!

@jeromekelleher
Copy link
Member

Let's update this and get it merged I think @GertjanBisschop - I guess we should try to register some issues for any remaining TODOs and corner cases so we don't forget them. We can update to the new API later.

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #2145 (5bf4057) into main (d05b851) will decrease coverage by 0.08%.
The diff coverage is 59.45%.

❗ Current head 5bf4057 differs from pull request most recent head b64100c. Consider uploading reports for the commit b64100c to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2145      +/-   ##
==========================================
- Coverage   91.45%   91.38%   -0.08%     
==========================================
  Files          20       20              
  Lines       11170    11199      +29     
  Branches     2268     2280      +12     
==========================================
+ Hits        10216    10234      +18     
- Misses        521      529       +8     
- Partials      433      436       +3     
Flag Coverage Δ
C 91.38% <59.45%> (-0.08%) ⬇️
python 98.74% <ø> (ø)

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

Impacted Files Coverage Δ
lib/msprime.c 85.89% <59.45%> (-0.18%) ⬇️

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 d05b851...b64100c. Read the comment docs.

@GertjanBisschop
Copy link
Member Author

GertjanBisschop commented Feb 24, 2023

Not sure how I might have reduced code coverage that significantly. What do I do to fix this?

@GertjanBisschop
Copy link
Member Author

Right I guess this means I'll have to write some additional tests in C.

@jeromekelleher
Copy link
Member

Let's have a look through this tomorrow @GertjanBisschop - it might be simpler to push through the final API rather than getting the C test coverage up.

@GertjanBisschop
Copy link
Member Author

Implemented in #2176.

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.

None yet

3 participants