-
Notifications
You must be signed in to change notification settings - Fork 111
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
Add user-defined category-label colors #1402
Conversation
Re. eslint errors: I don't think it makes sense to resolve those as part of this PR, since they all have to do with old code not in the diff. Probably better to separate concerns an ship those changes in a separate PR. |
We should turn off this test until such time as it is adding signal, rather than just noise! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one broad concern: the actual color format is going to be fragile (ie, is an dependency on an anndata internal semi-documented feature, will have users manually stuffing info into it, etc). The error handling / checking in our code doesn't seem robust to that. Examples:
- cxgtool - just fails without clean error message. There is no ability to force cxgtool to ignore the colors and convert anyway, etc. At a minimum, we need a command line param for cxgtool that turns this feature on/off, like we have for all other optional metadata (title, about, etc).
- anndata REST route - similarly, if this finds corrupt color data, it will cause the
cellxgene launch
to just fail (and without a useful error message). I think we need good error handling, and graceful degradation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great first cut. I think this primarily needs better consideration of error handling and how we deal with cases in which the color data model is unparsable, or the user does not want to display it.
- ability to enable/disable the use
- more graceful handling of badly formatted color table
Lots of other nits, but none of them are major.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this, @mweiden ! Do you have an example dataset file with specified colors for any of the categories that I could test with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mweiden If it is helpful, here is some JSON I made for the example-dataset
file.
It can go into it as a .cxg
, I just don't know how to create a .cxg file; also, my example-dataset is an .h5ad.
Also I would be happy to hop on a call to learn how and how to add or modify the cxg_group_metadata
section where this color info is intended to go (according to #1307 at least), and how to make a .cxg .
{
"louvain": {
"B cells:" "#D40C00",
"CD14+ Monocytes": "#FF9A00",
"CD4 T cells:" "#FFCD00",
"CD8 T cells:" "#CDE000",
"Dendritic cells": "#32C12C",
"FCGR3A+ Monocytes": "#00BCD9",
"Megakaryocytes": "#525EFF",
"NK cells": "#7F4Fc9"
}
}
It is really basic rainbow colors starting with red, (different from our defaults though), so should be easy to test with visually.
This is only used in the (automatically generated) CXG format. You want to learn how to manipulate the AnnData and add this to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is working for me in the UI
98ef166
to
9503201
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nits in cxgtool.py that I'd like to see fixed before merge. The docs issues are not yours, so feel free to fix or file a bug if not trivial to fix.
@mweiden - the one follow-up thought I had. We normally try to keep In this case, they have opposite defaults for colors:
This is new - all other features are aligned in behavior so that the defaults match. Thoughts? |
@bkmartinjr good points re. consistent CLI behavior. I've decided to make the feature on by default with consistent disable flags: both |
9503201
to
8fb9f70
Compare
Fixes #1152
Designed as described in #1307