search

biocore / LabControl

lab manager for plate maps and sequence flows
BSD 3-Clause "New" or "Revised" License
2 stars 15 forks source link

Test LC plating across multiple studies #437

Open charles-cowart opened 5 years ago

charles-cowart commented 5 years ago

Mackenzie and Gail have noted that, at least in the wet-lab, only a single study at a time has been test-plated. Before full-launch, we should test two or more studies being plated at the same time, as that is the most likely scenario.

fedarko commented 5 years ago

So... something worth noting here is that existing parts of the JS code apparently just assume there will only be one study. See, e.g., PlateViewer.commentWell(), which calls PlateViewer.getActiveStudy(), which in turn just assumes that only a single study is being plated.

It's weird, because some of the code looks like it's designed to handle multiple studies (e.g. autocomplete_search_samples(), which checks samples from every active study), but some of the code assumes that only a single study is being plated at a time. I'm not sure if LC has been successfully used to plate multiple studies in the past, but if not I'd wager that parts of the JS are the culprit.

charles-cowart commented 5 years ago

@fedarko Thanks for the catch! It sounds as though whoever added support for multiple studies in the PlateMapper didn't catch all of the areas that needed modification. This is definitely a good reason to comment and beautify code.

@fedarko @gwarmstrong Let's identify the instances where it looks like only one study is assumed, and we'll create a bulleted list in this issue documenting them all. We can then characterize with some certainty how big a job it will be to fix them all.

AmandaBirmingham commented 5 years ago

@fedarko @gwarmstrong I believe that the specific concern raised here about PlateViewer.getActiveStudy() is based on a misunderstanding. My understanding of that function is that it does not assume that a plate can only have samples from one study on it--rather, it assumes that the user can only add samples from one study at a time during the plating process. The studies listed in the plating interface act rather like radio buttons--there can be many of them, but only one can be selected (highlighted) at a time, and the one that is highlighted is considered the "active study". (Note this also implicitly assumes that a sample can only ever belong to one study, but AFAIK that is an assumption that comes from Qiita so we don't have a choice about it.)

We have done basic testing of the interface with multiple studies (using maptest.ucsd.edu--which appears to be down at the moment; does anyone know why?) However, more in-depth testing of that functionality is certainly warranted, especially as the plating code has grown more complex over time. For example, if a user adds samples from one study, then choses a different "active study" and adds samples from it, then goes back and adds a comment on a well containing a sample from the first study, which study id does the code in PlateViewer.prototype.commentWell() grab via PlateViewer.getActiveStudy()? Eww, we'd better check ...

fedarko commented 5 years ago

@AmandaBirmingham Thanks for the comment. I agree, I think I have misunderstood the scope of this problem—my apologies. (Also, I should apologize to @ElDeveloper—he raised this issue to me a few days ago in response to my comment on GitHub, and I assumed that he was talking about an older version of LabControl before the supposedly breaking changes were added. I should've looked into that more.)

So things are not as dire as I thought they were, which is likely better than the alternative :)

...Part of my confusion, I think, stems from get_active_studies():

https://github.com/biocore/LabControl/blob/1427ad163682ff11b1086e0e3b1010ba1a2166ee/labcontrol/gui/static/js/plateViewer.js#L774-L790

I added a second study to my local copy of Qiita recently and I've been playing around with it / LabControl. (Thanks @gwarmstrong for pointing me in this direction.) From what I've seen, if both studies are "added" then—in accordance with what Amanda described above—only the one selected study is considered "active." Since only one study can be active at a time, $studies can only contain at most one element. So most of the time get_active_studies() only returns an array containing a single study ID, which is not at all what I thought it was doing.

The only case in which multiple study IDs are returned by get_active_studies() is if multiple studies are present but neither is "active" (I just realized a few minutes ago that you can de-select added studies by clicking on them again, which is how you'd recreate this state). This is handled in the first branch of get_active_studies()' if-statement, but this seems... pretty counterintuitive to me? If only "selected" studies are considered active, then saying that all studies are active if none are selected seems funky (but I don't have a ton of experience with LabControl, so I may be out of my depth here).

I'll revisit this next week, but I agree that more testing with multiple studies would be a good idea. It also might be worth rewording/refactoring some of the code related to get_active_studies(), since the current implementation is (IMO) pretty confusing.

Sorry again for the misunderstanding—looking forward to moving towards making this code more and more robust :)