-
Notifications
You must be signed in to change notification settings - Fork 11
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
Synchronize flowCore_PnRmax handling between cytolib and flowCore FCS parsers #42
Conversation
Notes from offline discussion on the usage of the "transformation" and "PnRmax" keywords: It seems like the ideal logic was this:
However, that does not seem to be consistently applied. In particular, there are a couple of issues:
|
It looks like the main issue here might just be that When called on a flowWorkspace::transform(cytoset) -> flowCore::transform(flowFrame) -> flowCore::updateTransformationKeywords For The quick solution would probably be to just add the assignment of the keyword at the R level for Along the way, it would probably be a good idea to make a dedicated |
@mikejiang, @gfinak , based on the points in the comment above, to resolve the inconsistences and improve efficiency, I propose:
This would be instead of the original changes in this PR. |
Dedicated |
42952b5 and RGLab/flowWorkspace@0ea4fe5 change behavior so:
|
@jacobpwagner, does this mean that data transformations will get a speed boost too? |
@DillonHammill , possibly, with a few caveats:
|
Great thanks @jacobpwagner! |
@DillonHammill , it would seem there is a noticeable speedup. Not all that surprising I suppose, but good confirmation:
The timing output
|
That's excellent @jacobpwagner! The transformations are by far the most computationally taxing steps for me, so any speed improvements will not go unnoticed! |
See RGLab/flowWorkspace#348 for discussion of the different
flowCore
andcytolib
handling offlowCore_PnRmax
values in the FCS keywords. This commit is intended to rectify that, but because it involves a low-level change in the parser I thought it might be good to allow review/veto before merging it in. After this commit, allflowCore
,flowWorkspace
, andCytoML
tests (including internal) still pass, but the handling of instrument range is now more consistent betweenflowFrame
andcytoframe
: