Skip to content
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

Allowing reading for BD Accuri C6 and Attune files #13

Merged
merged 6 commits into from
Oct 3, 2022

Conversation

vogu66
Copy link
Contributor

@vogu66 vogu66 commented Sep 12, 2022

We added the ability to read floats in addition to integers and we fixed a problem with BD Accuri files where two offsets for an optional section were not defined. We can also provide test file for either machine.

@tlnagy
Copy link
Owner

tlnagy commented Sep 15, 2022

Thank you @vogu66, could you please add tests?

Unfortunately, it also looks like I never got around to updating the testing infrastructure for this package. It still uses Travis CI, which has been broken for awhile. At some point, I'll need to update it to use Github Actions. In the meantime, could you verify that the tests pass locally?

@vogu66
Copy link
Contributor Author

vogu66 commented Sep 16, 2022

Sure, I'll get around to seeing how that works. I just tried to access the urls in the test.jl file and they seem unavailable.

Maybe we can use files from the python package (https://github.com/cytoflow/fcsparser) for some of the testing, though I know some of them can't be read in Julia yet.

Do you want me to also add the files for which I fixed things, and new tests, or just the old tests? Edit: Nevermind, I hadn't read correctly. They will be added.

@LucasH1997 you can also have a look at it if you've got the time.

@vogu66
Copy link
Contributor Author

vogu66 commented Sep 28, 2022

@tlnagy the testing files that were online aren't accessible at those addresses anymore. I replaced one with a link to a file from the python parser (https://github.com/eyurtsev/fcsparser/blob/master/fcsparser/tests/data/FlowCytometers/FACSCaliburHTS/Sample_Well_A02.fcs) but the large file from the python parser can't be read from julia (even before my changes). Do you have working links towards the files?

@tlnagy
Copy link
Owner

tlnagy commented Sep 29, 2022

I believe the links were added by @laurentheirendt and @exaexa (and hosted on their infrastructure). Unfortunately, I don't have access to them separately.

the large file from the python parser can't be read from julia (even before my changes). Do you have working links towards the files?

Which file is that?

@tlnagy
Copy link
Owner

tlnagy commented Sep 29, 2022

So once I merge #14, we'll have CI back up and running so I'll need you to rebase this PR on master and then we can make sure the tests are working.

@exaexa
Copy link
Contributor

exaexa commented Sep 29, 2022

I believe the links were added by @laurentheirendt and @exaexa (and hosted on their infrastructure).

These are the links to flowrepository.org contents, right? That's (unfortunately) not our infrastructure, and (unfortunately) we have the very same download problems with them. I'll try to find a replacement.

You can get the files manually from here: https://flowrepository.org/experiments/4/download_ziped_files (flowrepo disabled simple downloading of single files, I guess because of bandwidth problems)

@tlnagy
Copy link
Owner

tlnagy commented Sep 29, 2022

These are the links to flowrepository.org contents, right? That's (unfortunately) not our infrastructure, and (unfortunately) we have the very same download problems with them. I'll try to find a replacement.

Yeah, looks like it:

r = HTTP.request("GET", "https://flowrepository.org/experiments/4/fcs_files/326/download", response_stream=io)

Might be possible to load them in a separate github repo and do a submodule clone, we do something like that over in OMETIFF.jl: https://github.com/tlnagy/OMETIFF.jl/blob/befd686f29e3cf4355e9bc1e7cc153667a68d9ed/test/runtests.jl#L22-L33

@vogu66
Copy link
Contributor Author

vogu66 commented Sep 30, 2022

Might be possible to load them in a separate github repo and do a submodule clone, we do something like that over in OMETIFF.jl: https://github.com/tlnagy/OMETIFF.jl/blob/befd686f29e3cf4355e9bc1e7cc153667a68d9ed/test/runtests.jl#L22-L33

That might actually be for the best for the module, as the tests are downloaded with it. When there will be more test files we don't really need everyone to have all that data on their computer, because if every package did that Julia would get very storage-hungry.

@vogu66
Copy link
Contributor Author

vogu66 commented Sep 30, 2022

So once I merge #14, we'll have CI back up and running so I'll need you to rebase this PR on master and then we can make sure the tests are working.

I think that's done

@vogu66
Copy link
Contributor Author

vogu66 commented Sep 30, 2022

It doesn't pass the tests I added because it doesn't find the files, but locally, when I comment out the original "big file" test, it works again, I'm guessing it's changing the directory with the error somehow.

Copy link
Owner

@tlnagy tlnagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to update the PR, I've left a couple minor comments below.

Project.toml Outdated Show resolved Hide resolved
src/parse.jl Outdated Show resolved Hide resolved
src/parse.jl Show resolved Hide resolved
@tlnagy
Copy link
Owner

tlnagy commented Oct 1, 2022

Once I merge #16, several example dataset will now live over in https://github.com/tlnagy/fcsexamples and will be downloaded only when you run the tests. There's an example Accuri C6 fcs file in that repo.

Do you mind rebasing this PR once I merge #16 again? I think it'll be ready to merge then.

@exaexa
Copy link
Contributor

exaexa commented Oct 2, 2022

several example dataset will now live over in https://github.com/tlnagy/fcsexamples and will be downloaded only when you run the tests

This is great, thanks!

@vogu66
Copy link
Contributor Author

vogu66 commented Oct 3, 2022

should be good, I think

I guess it should also more or less fix #4, although I consider in every case 32 bit integers here, and not variable length as in the standard.

@vogu66 vogu66 requested a review from tlnagy October 3, 2022 14:46
@tlnagy tlnagy merged commit 222be70 into tlnagy:master Oct 3, 2022
@tlnagy
Copy link
Owner

tlnagy commented Oct 3, 2022

Thanks for getting this done! I'll push this out as part of the v0.1.6 update.

@vogu66
Copy link
Contributor Author

vogu66 commented Oct 3, 2022

Thanks for getting this done! I'll push this out as part of the v0.1.6 update.

Well, thanks for the rest of the package

@tlnagy
Copy link
Owner

tlnagy commented Oct 3, 2022

Didn't realize I accidentally skipped a release, this'll be part of the v0.1.5 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants