individual properties `tag`, `tagF`, etc. should be persisted in .trees metadata

[bhaller]

It feels like it is time for this to happen, perhaps for SLiM 6.0. "How do I get my tag values in Python?" is an FAQ, the amount of on-disk space needed is not large (since the number of individuals in the individuals table is typically not large), the runtime overhead would be small (since the time/memory only needs to be spent at write time, and the memory usage is recovered as soon as the write is done), etc. So this issue is about adding this metadata. @petrelharp

However, doing this is a bit tricky. The reason is that all of the tag properties keep track of whether they have been given a value by the user or not. If not, and the user tries to access the tag value, they get a runtime error, "used before set", rather than getting random garbage or zero. This functionality has proved quite useful; it often catches bugs in the way that tag values are being used in a SLiM script. So we want to persist this "has been set" information on the Python side, and ideally we should even provide the same "used before set" error on the Python side, connecting the SLiM and Python sides with the same error-checking.

In SLiM's code, here's the declaration of the tag values in individual.h:

	unsigned tagL0_set_ : 1;			// T if tagL0 has been set by the user
	unsigned tagL0_value_ : 1;			// a user-defined tag value of logical type
	unsigned tagL1_set_ : 1;			// T if tagL1 has been set by the user
	unsigned tagL1_value_ : 1;			// a user-defined tag value of logical type
	unsigned tagL2_set_ : 1;			// T if tagL2 has been set by the user
	unsigned tagL2_value_ : 1;			// a user-defined tag value of logical type
	unsigned tagL3_set_ : 1;			// T if tagL3 has been set by the user
	unsigned tagL3_value_ : 1;			// a user-defined tag value of logical type
	unsigned tagL4_set_ : 1;			// T if tagL4 has been set by the user
	unsigned tagL4_value_ : 1;			// a user-defined tag value of logical type
	
	slim_usertag_t tag_value_;			// a user-defined tag value of integer type
	double tagF_value_;					// a user-defined tag value of float type

For tag and tagF there are special values used to indicate the "unset" state:

#define SLIM_TAG_UNSET_VALUE	(INT64_MIN)		// for tags of type slim_usertag_t, the flag value for "unset"
#define SLIM_TAGF_UNSET_VALUE	(-DBL_MAX)		// for tags of type double (i.e. tagF), the flag value for "unset"

In both cases, these are the minimum representable values for their respective data types (uint64_t and double). SLiM checks for these values on a "get":

			slim_usertag_t tag_value = tag_value_;
			
			if (tag_value == SLIM_TAG_UNSET_VALUE)
				EIDOS_TERMINATION << "ERROR (Trait::GetProperty): property tag accessed on trait before being set." << EidosTerminate();

(It really ought to check for them on a "set" and not allow them, also, but at present it doesn't; in practice it hasn't ever come up.)

For tagL0, tagL1, tagL2, tagL3, and tagL4, the scheme is a bit different. Taking tagL0 as an example, the tagL0_set_ data member of the Individual class tracks whether that tag value has been set or not, and the tagL0_value_ data member tracks the actual value of that tag value. So the initial state has the set flag false, a "get" checks for set false and errors, and a "set" makes the set flag true. So all of that works as expected, but I'm spelling it out a bit to be quite explicit about the model being followed by SLiM, which ought to be followed on the Python side as well.

So I'm not sure exactly how we want to handle persisting this. For tag and tagF we can probably just persist the value as it is; since individual metadata uses the struct codec the special "unset" marker values should get persisted exactly verbatim. For the tagLX values do we persist it as two separate boolean values? Or as a tri-state (unset, false, true) value?

And for the end user on the Python side, since we want to provide the same "error on get if not set" functionality, we probably don't want the user to be able to see the raw metadata values for any of this; we want to wrap those raw values in some API that handles the "get when unset" functionality, and hide the raw metadata. Maybe. Or maybe we just document this, and let users shoot themselves in the foot if they don't follow the rules.

Pyslim will need to worry about all this. When adding metadata annotations to individuals, their tag metadata should be initialized to "unset", presumably; and there should be some supported way of changing that annotation to a particular value, etc.

So it's a bit of a can of worms, and I think if it's going to happen I need a bit of guidance from you @petrelharp on how you'd like it to be designed. :-> Thanks!

[petrelharp]

Hm. It seems kinda straightforward: why not just record in metadata two booleans, one for the value and one for set, for each of the tagLs?

As for tagF - I would have picked one of the IEEE float NaN values to mean "unset". Maybe it's a good time to make that change? (And, too bad there isn't an int nan.)

[bhaller]

Hm. It seems kinda straightforward: why not just record in metadata two booleans, one for the value and one for set, for each of the tagLs?

Because that does not provide the desired semantics, unless the user themself uses the "set" flag according to those semantics (which is highly error-prone, and defeats the whole purpose of the "set" flag, which is to catch such semantics errors on the part of the user. I.e., it would allow the user to read the "value" bool even though "set" is false, with no error; and it would allow them to set the "value" bool without changing "set" to true; and so forth. See what I wrote above.

As for tagF - I would have picked one of the IEEE float NaN values to mean "unset". Maybe it's a good time to make that change?

I don't think it makes much difference in practice; nobody is ever going to have a calculation that produces exactly -DBL_MAX. And it would complicate things all over the code, in that I could no longer simply check for NaN with std::isnan(); I'd have to worry about whether it was the particular "unset" NaN.

(And, too bad there isn't an int nan.)

Indeed.

[petrelharp]

Sorry, I was going with your suggestion that the desired semantics would be implemented in python; if so, then this storage scheme seems like obviously the way to go.

Thinking about it more though: pyslim doesn't have anything to do with decoding the metadata. That is tskit. We could store the metadata somehow and have a pyslim helper function to decode it in the way you want, but people would then have to use the helper function. That doesn't seem better to me than just providing them "set/unset" and "value" and letting them sort it out. So, what you're asking for is a new binary metadata in tskit that is (somehow) "boolean or missing". References on how this works:

• list of supported binary types
• these are basically those supported by struct
• Booleans take one byte, and they are mapped to the ? code via this route

So one option would be to, say, store a char that is 0=False 1=True 2=unset; and then implement an encoder and decoder in tskit that did this conversion. That'd take a fair bit of mucking around in the metadata code, which I'm not enthusiastic about, tbh.

[bhaller]

Sorry, I was going with your suggestion that the desired semantics would be implemented in python; if so, then this storage scheme seems like obviously the way to go.

Ah, I see. Yes, for the underlying storage this seems fine. The question is about the user-facing semantics.

Thinking about it more though: pyslim doesn't have anything to do with decoding the metadata. That is tskit. We could store the metadata somehow and have a pyslim helper function to decode it in the way you want, but people would then have to use the helper function. That doesn't seem better to me than just providing them "set/unset" and "value" and letting them sort it out.

Hmm. It does seem better to me. As long as they use the pyslim helpers, they get the checking and the semantics that are desired, and it catches errors in their logic for them. If they do things on their own, they will inevitably make errors, leading to bugs and slim-discuss questions. :-> Wrappers that enforce required semantics are generally a good thing, I think...? It would be OK for the underlying set/unset and value stuff to still be visible directly in the tskit-vended metadata; if people use that, they're on their own. But if they use the pyslim getter/setter wrappers instead, they get an extra layer of safety that is recommended. What's the objection to that?

So, what you're asking for is a new binary metadata in tskit that is (somehow) "boolean or missing". References on how this works:

• list of supported binary types
• these are basically those supported by struct
• Booleans take one byte, and they are mapped to the ? code via this route

So one option would be to, say, store a char that is 0=False 1=True 2=unset; and then implement an encoder and decoder in tskit that did this conversion. That'd take a fair bit of mucking around in the metadata code, which I'm not enthusiastic about, tbh.

I agree that this sounds like a rat's nest, upon reflection. But pyslim wrappers seem pretty straightforward...? Or am I underestimating the difficulty?

[petrelharp]

I agree that this sounds like a rat's nest, upon reflection. But pyslim wrappers seem pretty straightforward...? Or am I underestimating the difficulty?

Let's see. pyslim wrappers on the two booleans would be straightforward. I'm not so enthusiastic because (1) I feel like saying "okay most metadata you get like ind.metadata['foo'] but for tag Li you probably want to use this special wrapper pyslim.logical_tags(ind, i)" is kinda confusing; and (2) just saying "there's two tags, Li and Li_set, that tells you the value of tagLi (defaults to F if unset) and Li_set (which tells you if it's been set" is pretty straightforward; seems like anyone who's mucking around in python with the tags could deal with this. I guess if the use case is "we expect that all tags will be set if we use them, and want an error if some are not" then that makes sense, but I do still wonder how much people will use the wrapper versus just doing metadata['L0'].

Anyhow I think the implementation is straightforward (two logical tags) and we can discuss getting it into pyslim later.

[bhaller]

I agree that this sounds like a rat's nest, upon reflection. But pyslim wrappers seem pretty straightforward...? Or am I underestimating the difficulty?

Let's see. pyslim wrappers on the two booleans would be straightforward. I'm not so enthusiastic because (1) I feel like saying "okay most metadata you get like ind.metadata['foo'] but for tag Li you probably want to use this special wrapper pyslim.logical_tags(ind, i)" is kinda confusing; and (2) just saying "there's two tags, Li and Li_set, that tells you the value of tagLi (defaults to F if unset) and Li_set (which tells you if it's been set" is pretty straightforward; seems like anyone who's mucking around in python with the tags could deal with this. I guess if the use case is "we expect that all tags will be set if we use them, and want an error if some are not" then that makes sense, but I do still wonder how much people will use the wrapper versus just doing metadata['L0'].

That is one use case, yes; and also "we expect that if a tag gets set the set flag for it will also be set, and want to ensure that". It is pretty common for the tag usage in a model to be fairly complex – for a particular tag value to be defined only one one sex and undefined for the other sex, only for some age classes and not for others, etc. It is easy to make logic errors with this. Adding the "unset" checks for tags caught MANY bugs, including in my own scripts. It is a valuable facility. Agreed the story with your point (1) is not ideal; but I think it is still worthwhile. If some people don't use the wrapper, so it goes; that is of course why it'd be nice to have that additional logic in tskit itself, but I think we agree that that is a bridge too far. The people who access the metadata themselves accept that risk; and the people who use the wrappers get the extra semantics that those provide. We could name the metadata keys beginning with an underscore to indicate that they are "internal", and document that people shouldn't use them directly.

Anyhow I think the implementation is straightforward (two logical tags) and we can discuss getting it into pyslim later.

I feel pretty strongly that the wrappers should be added to pyslim, for the reasons above. Adding the metadata is a bit contingent upon this decision, I think, actually. Starting the names with an underscore (which, now that I've thought of it, seems like a good idea?) will be a bit odd if pyslim doesn't provide the wrappers; so for the naming it would be good to know whether the wrappers will be there or not. And writing the doc for this metadata similarly does depend upon whether pyslim will wrap them or not. So let's just make a decision. I urge that the decision is yes.

[petrelharp]

Well, I'm not very enthusiastic about the underscores because a common (and, good) pattern in python is "get all the values into a vector" and work with the vectors. This ends up being, for instance, much more efficient. People working that way will want to use the unwrapped metadata, to avoid getting errors. So, I don't think we want to discourage people from using them? If you think underscores, then okay, go for it - but the documentation is going to say "to pull out vectors, use the underscores, but beware and make sure '_L0_set' makes sense; to pull it out for individuals use this wrapper".

I could write a wrapper in pyslim that pulls out the vectors - except that there's not a standard way to represent "booleans with missing data" in python. (a masked numpy array, maybe? but these are not very common.)

As I say, though - you've got the info; up to you.

[bhaller]

OK, I think we've settled on a plan; metadata without underscores, doc that states the semantics of the "set/unset" vs. "value" flags and recommends pyslim accessors that implement the semantics for individual-level access and recommends following the semantics oneself for vector-level access. I'll put it in as soon as I have a chance.