-
Notifications
You must be signed in to change notification settings - Fork 85
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
new additional_nodes API #2162
new additional_nodes API #2162
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2162 +/- ##
=======================================
Coverage 91.45% 91.46%
=======================================
Files 20 20
Lines 11170 11214 +44
Branches 2268 2281 +13
=======================================
+ Hits 10216 10257 +41
Misses 521 521
- Partials 433 436 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
2a492a6
to
a966615
Compare
We probably need a more elegant solution for specifying the full_arg option using the class NodeType(enum.Flag):
RECOMBINANT = 1 << 17
COMMON_ANCESTOR = 1 << 18
MIGRANT = 1 << 19
GENE_CONVERSION = 1 << 21
PASS_THROUGH = 1 << 22
@classmethod
def full_arg(cls, model=None):
if model is None:
return cls.RECOMBINANT | cls.COMMON_ANCESTOR | cls.MIGRANT | cls.GENE_CONVERSION
elif model.lower() == "hudson":
return cls.full_arg()
else:
raise ValueError |
The |
I don't think |
Ahh great - I missed that point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @GertjanBisschop. Some quick comments on how to be type safe with fixed size types, which we need for the flags, because they go over 16 bits. Technically, int
types are only guaranteed to be 16 bits long (but will usually be 32).
Just annoying legacy C stuff.
lib/msprime.h
Outdated
@@ -430,7 +432,8 @@ int msp_set_simulation_model_sweep_genic_selection(msp_t *self, double position, | |||
int msp_set_start_time(msp_t *self, double start_time); | |||
int msp_set_store_migrations(msp_t *self, bool store_migrations); | |||
int msp_set_store_full_arg(msp_t *self, bool store_full_arg); | |||
int msp_set_store_unary(msp_t *self, bool store_unary); | |||
int msp_set_additional_nodes(msp_t *self, int additional_nodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the additional nodes argument is a flags value, we want it to be a fixed size and with an unsigned type (there can be confusion with the sign bit, otherwise). So, change the type here to uint32_t
.
msprime/_msprimemodule.c
Outdated
@@ -1784,7 +1784,8 @@ Simulator_init(Simulator *self, PyObject *args, PyObject *kwds) | |||
Py_ssize_t num_populations = 1; | |||
int store_migrations = false; | |||
int store_full_arg = false; | |||
int store_unary = false; | |||
int additional_nodes = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoyingly, the Python C API doesn't support fixed width types like uint32_t
. So, the simplest thing to do then is to use a type that's guaranteed to be at least this size, and to cast to uint32_t later. So,
unsigned long additional_nodes
We use k
for unsigned long in the Python C API
msprime/_msprimemodule.c
Outdated
@@ -1926,8 +1927,9 @@ Simulator_init(Simulator *self, PyObject *args, PyObject *kwds) | |||
goto out; | |||
} | |||
} | |||
msp_set_additional_nodes(self->sim, additional_nodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here then we do
msp_set_additional_nodes(self->sim, (uint32_t) additional_nodes);
LMK when this is ready for a final look @GertjanBisschop |
Ready for a final look @jeromekelleher. I hope the idling lint tests will run again after a squash and rebase. |
The lint failing needs a PR to fix. I'll get to it. |
@Mergifyio rebase |
✅ Branch has been successfully rebased |
e6bb7d2
to
72c3aea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just need to remove the new parameters (all of them) from simulate
, and update the CHANGELOG and we're done.
Can you follow up with some issues to track what needs to be done next, and also scan to see if there's any we should close now as well after merging this PR?
72c3aea
to
048a3a5
Compare
Ready to go once CI is fixed - great work @GertjanBisschop! |
048a3a5
to
80cce06
Compare
First pass at new
additional_nodes
API for Hudson, SMC, SMC' and multi-merger coalescents.@benjeffery would you want to take a look at how I solved the
asdict
representation of as required for the provenance recording?EDIT: ignore that Ben. I think I figured it out. Is this the way to do it?