-
Notifications
You must be signed in to change notification settings - Fork 6
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
Implement FlowSample on top of AxisArrays #15
Conversation
The existing tests have been rewritten to take their test data from local files instead of attempting to download them --- the urls were broken. The large file test has been deleted. New tests have been added for: - the key-based indexing of channels, which is existing functionality - the key-based indexing of multiple channels, which is to be implemented - the integer indexing of individual samples, which is to be implemented - the collection of integer indexing of multiple samples, which is to be implemented
Multiple channels can be indexed using a Vector of Strings Individual samples can be indexed using an Integer Multiple samples can be indexed using a Vector of Integers
I realised I was just rewriting the implementation of AxisArray. This commit introduces the tests for a new FlowSample struct which is a simple wrapper for AxisArray with some params attached. AxisArray performance is supposed to be good, the interface doesn't have to change, and they can easily be converted to matrices as needed by GigaSOM.
These changes add lots of indexing options for FlowSamples. I have tried to keep backwards compatibility with the old FlowSample, I think we have it, but cannot be absolutely certain.
Awesome work @lgrozinger! Thanks for taking the time to do this. I left my inline notes above, I think the main things other than that are to update the documentation in the README. |
src/parse.jl
Outdated
end | ||
|
||
data = AxisArray(datamatrix, rows, collect(1:size(datamatrix)[2])) |
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.
Please use size(x, 2)
here instead of size(x)[2]
here and elsewhere
Ok I will make those changes you requested, thanks for your review and a great package |
Once I merge #16, do you mind rebasing this PR on master and use the new testing framework? The datasets are now downloaded from the https://github.com/tlnagy/fcsexamples repo when you run the tests. |
…0-01-00-09-49-374-1964714061 CompatHelper: add new compat entry for "AxisArrays" at version "0.4"
The axes of the FlowSample data are now called :param and :event size calls refactored to include dimension version bumped Base.axes redefined to call out to AxisArrays versions Tests for large file added back in
Iterate just forwards the method to AxisArrays
So now I just need to update the documentation in order to reflect these changes. Since we are bumping up to 0.2.0, maybe this is an opportunity to tighten up the interface a little. I just wanted to get your thoughts @tlnagy before doing so. I propose we make a getter for Access to the I suggest a |
Great idea
This is a good idea. We can throw a deprecation warning for data and params in
I think the My one concern about adding a Also, now that I merged #13, do you mind rebasing again? |
Ok I'll get to work on the first two, the deprecation and the I had not considered that final point. The thing is I'm pretty sure that Another idea --- if we let The downside is that we cannot handle a parameter in the FCS files called "$params", "params" (or probably "$PARAMS", "PARAMS" too). I don't see anything in the FCS3 spec called that so thats probably alright (unless they decide to add that in at some point in the future) |
Actually do we even need to offer anything like I imagine a workflow something like this julia> fcs = FileIO.load("test/fcsexamples/Day 3.fcs")
FlowSample{Float32, UnitRange{Int64}}
Machine: DVSSCIENCES-CYTOF-6.0.626
Begin Time: 09:59:46
End Time: 10:34:14
Date: 26-Jun-2014
Axes:
Time
Event_length
Cd108Di (Cd108Di)
Cd110Di (Cd110Di)
Cd111Di (Cd111Di)
Cd112Di (Cd112Di)
Cd113Di (Cd113Di)
Cd114Di (Cd114Di)
Cd116Di (Cd116Di)
Xe131Di
--- clipped ---
--- the display here is too long as is---
julia> fcs["Time", :]
1-dimensional AxisArray{Float32,1,...} with axes:
:event, 1:283706
And data, a 283706-element Vector{Float32}:
5.013
9.023
julia> fcs[["Time", "Xe131Di"], 1:10]
2-dimensional AxisArray{Float32,2,...} with axes:
:param, ["Time", "Xe131Di"]
:event, 1:10
And data, a 2×10 Matrix{Float32}:
5.013 9.023 11.172 12.409 15.221 21.706 30.82 53.385 60.091 68.424
0.984885 4.68169 5.07743 1.65703 0.0 2.20896 0.084262 0.0 0.0 1.95139
julia> Array(fcs[["Time", "Xe131Di"], 1:10])
2×10 Matrix{Float32}:
5.013 9.023 11.172 12.409 15.221 21.706 30.82 53.385 60.091 68.424
0.984885 4.68169 5.07743 1.65703 0.0 2.20896 0.084262 0.0 0.0 1.95139 But maybe for ease of use we can add support for all the basic Also is there any particular reason you put the parameters as the rows? I feel like the more intuitive thing, for me at least, is to have the events as the rows and the parameters as the columns. Because then we could add support for https://tables.juliadata.org/stable/#Implementing-the-Interface-(i.e.-becoming-a-Tables.jl-source) which make it super easy to do something like julia> DataFrame(fcs) # convert to DataFrames |
I would prefer to get rid of
I was weighing up the choice, but landed on this way round. It depends on what people are doing with the data but it might make a difference depending on how users are iterating over it (column or row order). In the end the difference might be really small, and I'm not sure which is going to work better.
I mean we can support that interface regardless, whatever order the axes go. It would be a really good feature I think. We can add it to this pull request if you like, or we can chop up these features and setup a "release" branch. Which we can merge these into separately. That way we can prepare a |
Okay, so then add an if statement in
That's true. Julia is column major so indexing is always faster column-wise: julia> strides(Array(fcs))
(1, 50) It's a question of if people are gonna going to index per-event or per-column. Honestly, it's probably fine as is. Per-event seems more likely, right?
That's fair. We can add that as a separate PR for version |
A warning is emitted when the user tries to access params or data directly. Tests for these warnings are also added.
FCS TEXT segment keywords can now be accessed using the dot syntax. E.g. `flowrun.par` retrieves the "\$PAR" keyword value. Dot syntax access to `data` and `params` throws deprecation warnings. `Base.propertynames` produces a list of the keywords for the `FlowSample`. This commit adds tests for these features also.
Hey @lgrozinger, what's the status of this PR? |
Hey sorry I was away for a while. Everything should be implemented as we discussed, with the exception of the Tables interface, which I think I will do in a new PR. The documentation however, is not updated to include the new indexing styles. The example in the documentation works, but I think it would be good to mention the new indexing capabilities there. I will try to get the docs done soon. |
Ok so I have added some docs in the readme. |
Sorry for the delay, but this looks great! I think it's ready to be merged and tagged. |
Looks like the version number won't work. Do you mind changing it to |
ah sorry |
Thanks for getting this done! |
Absolute pleasure, since I find this package very useful. |
AxisArrays is supposed to be an efficient implemented of the kind of data structure that
FlowSample.data
is --- a matrix of values with named rows.So I have reorganised
FlowSample
to use anAxisArray
instead of aDict
for its data. I think this has a bunch of advantages, especially because it opens up all new ways to index intoFlowSample
. Here are a few:The old channel access with
String
key still works:Multiple channels can be taken also in a sensible way:
Issue #12 is solved in a way:
As well as combinations of these indexing strategies. There are also more tests added in this PR, for these new features, and to check that the old indexing works still as expected.
The GigaSOM test suite still passes just fine with these changes, so I hope it shouldn't have an impact on GigaSOM users, though that assumption is only as good as the GigaSOM test suite.
More detail can be found in the commit messages.