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

2_pixie cluster pixel does not retrain SOM when new channel added #887

Closed
HPiyadasa opened this issue Jan 23, 2023 · 6 comments · Fixed by #903
Closed

2_pixie cluster pixel does not retrain SOM when new channel added #887

HPiyadasa opened this issue Jan 23, 2023 · 6 comments · Fixed by #903
Assignees
Labels
bug Something isn't working

Comments

@HPiyadasa
Copy link

Describe the bug
When new channel is added, pixel preprocessing generates "New channels provided: overwriting whole cohort" and reprocesses the data. However going into - create the pixel-level SOM weights section does not regenerate the pixel SOM with new channel and instead get this msg - "Pixel SOM already trained". This results in an error when using pixel SOM weights to assign pixel cluster step.

"1 value(s) provided for list pixel som weights columns and list pixel data columns are not found in both lists."

Expected behavior
Ability to add new channels and recluster
Screenshot 2023-01-23 at 11 32 34 AM
Screenshot 2023-01-23 at 11 32 45 AM
Screenshot 2023-01-23 at 11 32 57 AM

@HPiyadasa HPiyadasa added the bug Something isn't working label Jan 23, 2023
@ngreenwald
Copy link
Member

@cliu72 what's your preference for the user logic here. Should we update all of the subsequent steps to check if the user has made changes? Or have them create a separate folder and start over? Or some other alternative?

@cliu72
Copy link
Contributor

cliu72 commented Jan 23, 2023

Hm I think it would be nice to have a check if the user has made changes (as opposed to having the user create a whole new folder and starting over). It seems like the preprocessing function is already checking for changes to the channels, but it's actually not checking if other things (i.e. blur sigma, subset proportion) are changed. I think those three (channels, blur sigma, subset proportion) are the three we need to check for - what do you think? I think especially when people are testing (like Hadeesha is right now), it'd be helpful for people to be able to make changes to these parameters and have them propagate.

@ngreenwald
Copy link
Member

Sure, sounds good!

@alex-l-kong
Copy link
Contributor

@cliu72 for verifying channels we can just read in the existing weights and verify that the channels columns match up. Since blur_factor and subset_proportion are parameters in create_pixel_matrix completely independent of the PixelSOMCluster class, for now one easy fix (albeit not clean) would be to save these preprocess variables (blur_factor and subset_proportion) in a JSON file and use that to verify the parameters didn't change.

More programmatically speaking, we should have all-encompassing classes for the entire Pixie pipeline (one for pixels, one for cells) which adds create_pixel_matrix as a method for pixel clustering and maintains blur_factor and subset_proportion attributes, as well as attributes of type PixelSOMCluster and PixieConsensusCluster. This will be a longer term project.

@alex-l-kong
Copy link
Contributor

@cliu72 open to any other suggestions here too!

@cliu72
Copy link
Contributor

cliu72 commented Feb 2, 2023

I like your suggestion @alex-l-kong about verifying channels matches up with the existing weights. I also like eventually having all-encompassing classes for the entire Pixie pipeline - but I agree this is a longer term project and low priority.

After thinking about this some more, I think for now, we can just assume the user doesn't change blur_factor and subset_proportion (and just add that functionality of checking when we switch to the object-oriented version). I think the JSON option would work, but not worth the effort for something that isn't going to be used that often. Thanks Alex!

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

Successfully merging a pull request may close this issue.

4 participants