search

saeyslab / CytoNorm

R library to normalize cytometry data
33 stars 6 forks source link

Update needed for FlowSOM object update? #24

Closed jacobpwagner closed 3 years ago

jacobpwagner commented 3 years ago

Hi Sofie,

I could be wrong, but it seems like CytoNorm's internal handling of FlowSOM objects may need to be updated after the Big Update commit to FlowSOM reorganized the list structure.

If I install both packages from the current GitHub master branches (or FlowSOM from Bioconductor 3.13) and attempt to run through the examples from the README, I run into issues from testCV or CytoNorm.train. For example, from CytoNorm.train, I get one of these:

Error in UpdateFlowSOM(fsom) : fsom should be a FlowSOM object.

It's not hard to trace the cause of this to here, where fsom$FlowSOM will yield a NULL for the newer format fsom object generated by prepareFlowSOM. There are also a few other locations that seem to directly depend on the old FlowSOM object structure.

Please let me know if I'm just missing something. I could also probably put together a quick patch PR as this should be pretty straightforward.

Thanks, Jake

SofieVG commented 3 years ago

Hi Jake,

I just pushed the fixes I had locally, but they are not yet fully tested (the reason I kept postponing the push). Let me know if you still find any other bugs!

All the best, Sofie

On Fri, 4 Jun 2021 at 00:37, Jake Wagner @.***> wrote:

Hi Sofie,

I could be wrong, but it seems like CytoNorm's internal handling of FlowSOM objects may need to be updated after the Big Update https://github.com/SofieVG/FlowSOM/commit/ce17ea65760073e57dfffdc378f3f1305498fcd8 commit to FlowSOM reorganized the list structure.

If I install both packages from the current GitHub master branches (or FlowSOM from Bioconductor 3.13) and attempt to run through the examples from the README, I run into issues from testCV or CytoNorm.train. For example, from CytoNorm.train, I get one of these:

Error in UpdateFlowSOM(fsom) : fsom should be a FlowSOM object.

It's not hard to trace the cause of this to here https://github.com/saeyslab/CytoNorm/blob/e94732cec4ce26c44d167c530978b2c5b296e55c/R/CytoNorm.R#L252, where fsom$FlowSOM will yield a NULL for the newer format fsom object generated by prepareFlowSOM. There are also a few other locations that seem to directly depend on the old FlowSOM object structure.

Please let me know if I'm just missing something. I could also probably put together a quick patch PR as this should be pretty straightforward.

Thanks, Jake

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/saeyslab/CytoNorm/issues/24, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOS72ZCYDALIQIKUVE7KDTTQ773ZANCNFSM46BUN4LQ .

jacobpwagner commented 3 years ago

Thanks! It looked like there were just a few more simple changes needed in some of the evaluation functions. I put those in #25, just as an easy way to highlight them.

jacobpwagner commented 3 years ago

Sorry for the delay in closing this. I didn't see that you had merged that PR. Thanks again.