search

RGLab / openCyto

A package that provides data analysis pipeline for flow cytometry.
GNU Affero General Public License v3.0
77 stars 29 forks source link

Export .preprocess_csv #202

Closed DillonHammill closed 5 years ago

DillonHammill commented 5 years ago

Hi @mikejiang & @jacobpwagner,

Do you think that it would be possible to export .preprocess_csv? I need to use this in my package to expand the csv gatingTemplate. Unfortunately, I cannot use ::: to use this internal function within my package as it generates R CMD CHECK NOTEs. If I import it then I encounter the problem of having to import all the other internal functions that it uses (e.g. .split_multi_parents, .validity_check_alias, .preprocess_row and .unique_check_alias).

I could write my own version but I would like to keep it consistent with how the template is processed within openCyto.

Any help or suggestions would be greatly appreciated.

Thanks for your help! Dillon

jacobpwagner commented 5 years ago

Hey @DillonHammill. I talked it over with @mikejiang and we think it should probably not be exported. If we export it, then we have to document it and support external use (and probably re-name it as well) and it just wasn't designed to be user-facing, but rather was meant to stay an internal helper function. That said, your use case is very different from most users so occasionally hooking in to internal functionality is much more understandable. Are the NOTEs generated from R CMD CHECK an issue for rOpenSci? Can a brief explanation be included in the submission notes? I will gladly write/sign off on such an explanation.

gfinak commented 5 years ago

Just a hot take, what about exporting a wrapper specifically for cytorsuite_ functions that aren't otherwise exported? We could wrap .preprocess_csv, provide minimal docs and explicitly state it is to support cytorsuite. That's if NOTEs are a problem for rOpenSci.

jacobpwagner commented 5 years ago

Sure. I'd be alright with that. Given how closely interwoven CytoRSuite is with our packages, I think that makes sense. I would probably just downplay/omit them from the coming doc website so they don't confuse regular users.

gfinak commented 5 years ago

I agree

DillonHammill commented 5 years ago

Thanks for your help, really appreciate it!

I am in the process of finalising the CytoRSuite which will be officially released as CytoExploreR in the next week. This will permanently replace CytoRSuite. I am planning to submit to both ROpenSci and BioConductor so I think I need to remove any R CMD CHECK NOTEs.

Wrapping and exporting allowing export of this function as CytoExploreR_ would be greatly appreciated!

I have not encountered any other import issues, I think this is the only one.

Thanks again!

jacobpwagner commented 5 years ago

Sounds good. I'll get on it. There were a few others right? Could you give me a list of internal functions (across all necessary packages) it would help to low-key export in this manner so I can do it all at once?

DillonHammill commented 5 years ago

Thanks @jacobpwagner! I am currently away from my computer but I will prepare a list this afternoon so that it is ready for you tomorrow morning.

DillonHammill commented 5 years ago

@jacobpwagner, here is complete list of the internal functions that I am using from the RGLab suite in CytoExploreR:

flowCore

openCyto

It does not look like I have used any internal functions from flowWorkspace.

I can import .estimateLogicle and .argDeparser using utils::getFromNamespace which bypasses the R CMD CHECK NOTEs for these functions. This does not however work for .preprocess_csv as it uses a series of additional internal functions. If you would prefer, I can continue to use the getFromNamespace workaround for .estimateLogicle and .argDeparser, which would just leave .preprocess_csv for this low-key export?

I guess the nice thing about exporting them as CytoExploreR_ it that it will flag any functions that I am using in the event that you need to make any changes to them.

jacobpwagner commented 5 years ago

I was thinking the same thing. It will probably help to export all of them this way so we'll know if any of our changes have the potential to disrupt your code so we could give you a heads up. So you shouldn't need the utils:::getFromNamespace workaround.

Thanks! I'll take care of this today.

jacobpwagner commented 5 years ago

Done in e035cbfb8dd979cc5d3dd431ed376b01ab90ec2b and RGLab/flowCore@7abe8d0a25f4eebaa09374692b28fda2c55f407c. @DillonHammill Will you make sure they work as expected for your needs and then I'll go ahead and close this.

DillonHammill commented 5 years ago

Thanks @jacobpwagner! Everything is working on my end. Really appreciate this!