LTLA / scuttle

Clone of the Bioconductor repository for the scuttle package.
https://bioconductor.org/packages/devel/bioc/html/scuttle.html
9 stars 7 forks source link

makePerCellDF using alt rowData Column #18

Closed samuel-marsh closed 1 year ago

samuel-marsh commented 1 year ago

Hi,

Still learning my way around SCE class so apologies if this is obvious question. Using makePerCellDF is there a way to specify that features parameter should be searched for in rowData similar to the swap_names parameter in scater:::retrieveCellInfo(x = sce, by = FEATURE, swap_rownames = "Symbol")?

Thanks! Sam

LTLA commented 1 year ago

Not at the moment. Probably not too hard to implement by adding a swap.names= argument to .harvest_se_by_column.

I don't have a lot of time at the moment, but if you like, you can get the ball rolling with a PR.

samuel-marsh commented 1 year ago

Sounds good! Ya it’s easy addition by porting .swap_rownames and .get_rowData_column from scater and then adding param to .harvest_se_by_column and makePerCellDF.

I will submit PR shortly. I also have PR I’ll push for scater as retrieveCellInfo(…, swap_rownames) was only checking alt rowData names in main exp and not in altExp(s).

One logistical question for you before I push PR. I noticed in makePerCellDF that colData is added to the output after feature data for main exp and then altExp. This results in slight output format discrepancy:

This could obviously be resolved if colData was added to output first, then main exp feature data, then altExp feature data. However, before switching that order I wanted to check whether or not current format was intentional design choose to separate features in output if user is trying to retrieve both main exp and altExp data at same time? If so then I will just leave that as.

Thanks! Sam

LTLA commented 1 year ago

Thanks, done and pushed.

As for the order, no reason; I probably added the altexps later, so the observed order is just a consequence of development history. Anyway, I ended up moving the main exp feature data behind the reduced dims so that they sit alongside the alt exp features.

Technically the order change is a breaking change, but I doubt anyone was using the result of this function in an order-sensitive way, so I just pushed to BioC-release as it is.

samuel-marsh commented 1 year ago

Awesome! Thanks so much for quick replies and push!!

Best, Sam