cortex-lab / alyx

Database for experimental neuroscience laboratories
44 stars 11 forks source link

Refactory zygosity logic #430

Closed rossant closed 5 years ago

rossant commented 6 years ago

Rules are currently hard-coded ; they should be part of the database definition instead. We may need to revisit the schemas. This is related to a bug reported by @charureddy, the correction of which might be the occasion to rethink the way it's currently implemented. cc @kdharris101 @nsteinme

kdharris101 commented 6 years ago

Let’s discuss on the next call. (I’m not here this week though.)

From: Cyrille Rossant notifications@github.com Sent: 08 August 2018 09:09 To: cortex-lab/alyx alyx@noreply.github.com Cc: Harris, Kenneth kenneth.harris@ucl.ac.uk; Mention mention@noreply.github.com Subject: [cortex-lab/alyx] Refactory zygosity logic (#430)

Rules are currently hard-coded ; they should be part of the database definition instead. We may need to revisit the schemas. This is related to a bug reported by @charureddyhttps://github.com/charureddy, the correction of which might be the occasion to rethink the way it's currently implemented. cc @kdharris101https://github.com/kdharris101 @nsteinmehttps://github.com/nsteinme

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cortex-lab/alyx/issues/430, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AD7KlTnmGUO8JccN-sDyWRxGy6NaUm0vks5uOuMFgaJpZM4Vz5ty.

rossant commented 6 years ago

What we'll need to define clearly is the algorithm specifying (1) when the zygosities are set and (2) how they are set, depending on the line, the sequences, the genotype tests, the parents genotypes, including when the parents are from different lines. It seems this algorithm is not clearly defined right now and the code is behaving incorrectly in some cases.

charureddy commented 6 years ago

Thanks Cyrille. We have to resolve this soon, as currently, users have no idea whether an animal is negative or positive, and this can end up in them picking the wrong animal.

Regards, Charu.

On 8 August 2018 at 14:49, Cyrille Rossant notifications@github.com wrote:

What we'll need to define clearly is the algorithm specifying (1) when the zygosities are set and (2) how they are set, depending on the line, the sequences, the genotype tests, the parents genotypes, including when the parents are from different lines. It seems this algorithm is not clearly defined right now and the code is behaving incorrectly in some cases.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cortex-lab/alyx/issues/430#issuecomment-411412125, or mute the thread https://github.com/notifications/unsubscribe-auth/AbP-XDkGyQSBAQDN4gCOFtWoWLe1HtKeks5uOuyAgaJpZM4Vz5ty .

-- Charu Reddy Laboratory Manager Cortical Processing Laboratory University College London The Cruciform Building Gower Street London WC1E 6BT Phone: 020 7679 6147 https://www.ucl.ac.uk/cortexlab

nsteinme commented 6 years ago

Hi Cyrille, I just spoke with Charu about this. The reason for the unexpected behavior was that some new lines had not had their zygosities rules set up in zygosities.py yet. I don't think there is any problem with our current approach in terms of algorithm definitions, but we should make zygosities.py into a table so that charu/others can easily see it and update it.

I think this is simple, and we should do it asap. 1) A new table called ZygosityRules. 2) Each row of the new table has: line, allele, sequence0, sequence0result, sequence1, sequence1result, zygosity. This specifies: if a mouse is from that line, and has sequence0result for sequence0, and sequence1result for sequence1, then the zygosity of the allele should be set as specified. sequence1 can be empty. This is exactly the same information as in zygosities.py.

Every time any mouse gets a new sequence result, this search is run for that mouse's line and sequences, and zygosities are updated. I believe this is exactly the current logic and as mentioned, the current problems Charu has noted are not related to that logic, but instead related to the fact that the zygosity rules were invisible to her.

Is it easy to set this up on alyxdev? If you think we can't get it up today or tomorrow then I'll make a PR with changes to the existing zygosities.py, but I think there's no reason to put this off....

rossant commented 6 years ago

thanks! I'll try to do that this afternoon

rossant commented 6 years ago

should be done on alyx-dev, see https://alyx-dev.cortexlab.net/admin/subjects/zygosityrule/?all=

please double-check that it works correctly...

nsteinme commented 6 years ago

Looks awesome! The interface for adding them is great and the table fields all look correct. I made a test rule and gave a test mouse a new sequence that fit the test, and it worked. Any other parts of the mechanism that you're thinking need testing? I'll go over it briefly with Charu tomorrow too and we'll get back to you.

nsteinme commented 6 years ago

Whoops, Charu and I just tested this and it seemed not to be working this morning. We tried adding a new rule and then a new sequence for a test mouse, but the mouse's zygosity did not update. We also tried changing a rule and zygosities did not update for existing mice. What determine when the rules are applied? And how are you determining precedence of inferring zygosity from parents versus this table of rules?

rossant commented 6 years ago

We tried adding a new rule and then a new sequence for a test mouse, but the mouse's zygosity did not update.

Did that work yesterday?

We also tried changing a rule and zygosities did not update for existing mice.

Indeed, this has not been implemented, should it?

What determine when the rules are applied?

nsteinme commented 6 years ago

I thought it did work yesterday. But it's not working now, I just tried again. So e.g. I added a rule for line Ai32cg.Pv and allele Ai32-ChR2, and then gave a genotype test for the appropriate sequence to a mouse. But the zygosity did not update.

According to the rule, the mouse below should have gotten Ai32-ChR2 -/-, but as you can see, did not. This is after clicking "save and continue editing" on that subject, and I provide the subject link so you can check it out.

In this case I made the rule before adding the test result so I think it should have worked already. But I think if a rule is changed/updated/created then yes, it should update any mice to which the new rule applies. We at least need that for now as some rules should be back-applied to existing mice whose genotype test results are already in the database, and I can also imagine in the future that someone may make a mistake with the rule and want to fix it - after fixing it, they would expect that the new rule would be in force. The question is just whether there's any situation where you'd want to leave mice with the zygosity label they had under the old rule, when you make a new rule - I can't think of a reason you'd want to do that. One slightly tricky aspect may be: if a mouse originally had a determination from the parents about a zygosity, then that zygosity was overwritten by a genotype test rule - but then that zygosity rule is deleted. In that case, it should revert back to the zygosity from the parent. Perhaps the thing to do is: when a rule is updated/created/deleted, then for any mice which match that line and have that genotype test, the zygosity determination should be re-performed from the top: 1) from parents the subsequently 2) from rules. Is that hard to do?

image

https://alyx-dev.cortexlab.net/admin/subjects/subject/f1e9544a-0aea-4b46-8159-6e6cd50c448e/change/ image

rossant commented 6 years ago

According to the rule, the mouse below should have gotten Ai32-ChR2 -/-, but as you can see, did not.

Ah, but the zygosity already existed, so there's a "conflict"? In this case, the default behavior is to skip the rule, but I guess we should change that to enforce the new zygosity. What if you try with a zygosity that does not exist when you apply the rule?

I'll implement the other enhancements.

rossant commented 6 years ago

okay I pushed some changes on alyx-dev, could you try again?

nsteinme commented 6 years ago

K one observation, I see this now in the subject view: "Ai32-ChR2 +/- for Bovet; Pv-Cre +/- for Bovet" whereas before it was just " Ai32-ChR2 +/- ; Pv-Cre +/- ". The previous one was correct, there's no need for the subject name there.

On Fri, Aug 10, 2018 at 2:02 PM Cyrille Rossant notifications@github.com wrote:

okay I pushed some changes on alyx-dev, could you try again?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cortex-lab/alyx/issues/430#issuecomment-412075556, or mute the thread https://github.com/notifications/unsubscribe-auth/AHPUP_hyy5tJEZA-LSernY62Iy-a1DrXks5uPYRwgaJpZM4Vz5ty .

rossant commented 6 years ago

sure, it was helpful for debugging but I'll remove it

nsteinme commented 6 years ago

I see. Ok, adding a new rule works, and changing the rule reflects properly. Bovet's zygosity is still not updating according to the rule, though...? I even tried deleting the genotype test and re-adding it. Still shows Ai32-ChR2 allele as Het when it should be Absent, according to the new rule.

rossant commented 6 years ago

Ah, it was a bug with zygosity=0 (absent)... try again?

nsteinme commented 6 years ago

Ok. That's working. I'm now trying to figure out this mouse: https://alyx-dev.cortexlab.net/admin/subjects/subject/c9d48a99-d8b9-404a-8466-625a45e9270f/change/

If I add Cre test, then given the new rule the PVCre zygosity is getting updated correctly. But, when I remove the Cre test, it goes back to what it was originally, which is PVCre -/-. First thing is that I'm not sure why it had that value originally, since it had that value before I defined any rule about Cre for that line. Second is I'm not sure why it went back to that value.

It's not getting the value from the parents (or if it is, then it's a bug) because the PV parent was PV-Cre +/- (so there would be no conclusion about the offspring for this allele. Can you figure out why that mouse has PV-Cre -/-?

rossant commented 6 years ago

hmm I'm not able to reproduce this... :/ I'm going to push an update that makes it easier to navigate individual zygosity and genotype test instances. Maybe you could delete all objects related to this and try again. This is what I did locally and the bug didn't seem to appear.

rossant commented 6 years ago

done, see https://alyx-dev.cortexlab.net/admin/subjects/zygosity/ https://alyx-dev.cortexlab.net/admin/subjects/genotypetest/

you have some filters, and you can search by subject

nsteinme commented 6 years ago

I ran out of time and didn't get to test this more... @charureddy if you are able to test more on alyx-dev this weekend then maybe there's a possibility of a monday morning update? Otherwise we'll have to wait to test more next week, I'm gone this weekend.

charureddy commented 6 years ago

Sorry, I am away this weekend too, so I can only do it on Monday. Thanks Cyrille and Nick! Charu.

On 10 August 2018 at 18:52, Nick Steinmetz notifications@github.com wrote:

I ran out of time and didn't get to test this more... @charureddy https://github.com/charureddy if you are able to test more on alyx-dev this weekend then maybe there's a possibility of a monday morning update? Otherwise we'll have to wait to test more next week, I'm gone this weekend.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cortex-lab/alyx/issues/430#issuecomment-412157621, or mute the thread https://github.com/notifications/unsubscribe-auth/AbP-XK8lTaL9abxMPKBugazqHpaHf8cEks5uPchagaJpZM4Vz5ty .

-- Charu Reddy Laboratory Manager Cortical Processing Laboratory University College London The Cruciform Building Gower Street London WC1E 6BT Phone: 020 7679 6147 https://www.ucl.ac.uk/cortexlab

rossant commented 5 years ago

what's the status of this?