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

Revert site_full back to site #45

Closed
gwaybio opened this issue Aug 6, 2020 · 5 comments
Closed

Revert site_full back to site #45

gwaybio opened this issue Aug 6, 2020 · 5 comments
Labels
bug Something isn't working

Comments

@gwaybio
Copy link
Member

gwaybio commented Aug 6, 2020

In #23 (specifically #23 (comment)) I suggested a column name change within the 0.preprocess recipe module. This was a bad idea!

It was a bad idea b/c it breaks 0.merge-single-cells.py at: https://github.com/broadinstitute/pooled-cell-painting-profiling-recipe/blob/1be45bb33e1da71050dbb102352690cf8a08fb7c/1.generate-profiles/0.merge-single-cells.py#L143L146

we need to revert this change (and address some corresponding breaks after updating) before we can produce profiles.

@gwaybio gwaybio added the bug Something isn't working label Aug 6, 2020
@ErinWeisbart
Copy link
Member

ErinWeisbart commented Aug 6, 2020

Will we stick with nomenclature that site is the full string and Site is parsed?
e.g. site = 151_B1_12 and Site = 12
so site parses to Plate Well Site

@gwaybio
Copy link
Member Author

gwaybio commented Aug 6, 2020

We need to determine site nomenclature here and settle on what terms to use. Luckily, I think this is pretty straightforward, but it does require more thought that I had originally used.

The issue specifically is that, in #23, we updated a column name from site to site_full. After merging the CellProfiler compartments (using merge_single_cell_compartments()) we hardcode pd.assign() a metadata column called Metadata_Foci_Site here. After the update to site_full in #23, this line specifically makes it incompatible to merge the cell_ids (with metadata) to the CellProfiler features, given our current requirement for the columns to have a 1to1 mapping.


A side note that is beyond scope of this issue, is that I am noticing quite a lot of bleed through between non-core config entries in the two config files. I think streamlining these config files (maybe worth revisiting some of the decisions in #5). For example, I think we can solve the site_full issue with better config options. But this solution is beyond the scope of version 0.1 as well.

@gwaybio
Copy link
Member Author

gwaybio commented Aug 6, 2020

Will we stick with nomenclature that site is the full string and Site is parsed?

I think we should definitely stick to site being the full label (e.g. "151_B1_12"). This is consistent with our folder structure, and I think it is the path of least resistance to solving this bug. Also, I agree that site can parse to Plate, and Well, but I am not so sure about Site. The capitalization being the only difference is pretty fragile. For example, I can imagine wanting to standardize capitalization of all columns at some point, and building this in now would break that operation.

Currently, what do we use the parsed Site for now? Is it just QC visualization? If so, I propose that we keep the site to reference the full label, and then site_location to reference the physical location in the well. I think this will do for version 0.1, but we should address the issue the right way by using a config file instead of hardcoding for version 0.2. (the config files will need to be different than as they are now)

@gwaybio gwaybio added this to the Version 0.1 Release milestone Aug 7, 2020
gwaybio added a commit to gwaybio/pooled-cell-painting-profiling-recipe that referenced this issue Aug 7, 2020
gwaybio added a commit to gwaybio/pooled-cell-painting-profiling-recipe that referenced this issue Aug 7, 2020
this is still where the weld is broken, working through it now
@gwaybio
Copy link
Member Author

gwaybio commented Aug 7, 2020

In fb8eb0a I made an executive decision to do as proposed in #45 (comment)

in c8bbaaa i attempt to fix this in the qc step. This is still broken - will fix asap

@gwaybio
Copy link
Member Author

gwaybio commented Aug 10, 2020

in d98ded3 is a somewhat fragile fix... It works with the current setup, but we really need to create a much improved config system in version 0.2. I believe that an enhanced config system will deal with these inconsistencies automatically and "for free".

This fragile fix should be revisited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants