Closed zmar closed 5 years ago
Great, I hope to be able to review this during the afternoon. filter_features() does not need to know about the backend implementation, so it should be a plain old function. It is defined in FacileDataSet currently, but we will be moving it to the FacileAnalysis package.
Pete
Peter M. Haverty, Ph.D. Genentech, Inc. phaverty@gene.com
On Fri, Sep 14, 2018 at 8:16 AM, zmar notifications@github.com wrote:
@zmar https://github.com/zmar requested your review on: Genentech/FacileDataSet#19 https://github.com/Genentech/FacileDataSet/pull/19 Updated documentation for organism, samples, fds, set_fds, assay_info….
— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/Genentech/FacileDataSet/pull/19#event-1846376024, or mute the thread https://github.com/notifications/unsubscribe-auth/AH02Kzr9BdZmbBz-qJxN3m3XIR8POsPiks5ua8hkgaJpZM4Wpdrr .
Why would filter_features
be part of a FacileAnalysis
package? It's a "basic/fundamental" way to extract data from an object that implements a FacileInterface
, you know?
I would like to put filter_features() in FacileAnalysis. filter_features calls feature_Info_tbl(), which needs a specific implementation for each backend. We've got them for FacileDataSet and FacileExpressionPlotProject so far. These backend-specific functions go in e.g. FacileDataSet and the backend-agnostic things go in FacileAnalysis. Good?
Pete
Peter M. Haverty, Ph.D. Genentech, Inc. phaverty@gene.com
On Fri, Sep 14, 2018 at 9:53 AM, Steve Lianoglou notifications@github.com wrote:
Why would filter_features be part of a FacileAnalysis package? It's a "basic/fundamental" way to extract data from an object that implements a FacileInterface, you know?
— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/Genentech/FacileDataSet/pull/19#issuecomment-421419587, or mute the thread https://github.com/notifications/unsubscribe-auth/AH02K0WMMTI1m_7wp4CWR2vZpy3dhTR2ks5ua97-gaJpZM4Wpdrr .
Yes, I like it.
I still think the package at the top of the hierarchy should just be "FacileData", which defines the S3 generics that make the FacileData API as well as implementations o methods like filter_features
which can be written in this agnostic way that you mention.
FacileDataSet and FacileExpressionPlotProject would only depend/import that.
And FacileAnalysis would be where you put analysis moves (like PCA, dge, whatever) that leverage a datastore that implements the FacileData API, so again this FacileAnalysis package should only have to import a FacileData package.
yes/no?
Hi Steve,
The thing I'm calling FacileAnalysis is the same as what you are calling FacileData. This morning we were talking about moving the DGE, survival, etc. modules to separate packages. Many 3rd parties are keen to provide their own UI/analysis modules, which will have to be independent packages. We might as well make the existing ones follow the same pattern.
Pete
Peter M. Haverty, Ph.D. Genentech, Inc. phaverty@gene.com
On Fri, Sep 14, 2018 at 12:23 PM, Steve Lianoglou notifications@github.com wrote:
Yes, I like it.
I still think the package at the top of the hierarchy should just be "FacileData", which defines the S3 generics that make the FacileData API as well as implementations o methods like filter_features which can be written in this agnostic way that you mention.
FacileDataSet and FacileExpressionPlotProject would only depend/import that.
And FacileAnalysis would be where you put analysis moves (like PCA, dge, whatever) that leverage a datastore that implements the FacileData API, so again this FacileAnalysis package should only have to import a FacileData package.
yes/no?
— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/Genentech/FacileDataSet/pull/19#issuecomment-421459725, or mute the thread https://github.com/notifications/unsubscribe-auth/AH02K9vSzu6nZLZ_e3FNwXSWzt-RQiKIks5ubAIogaJpZM4Wpdrr .
Yes, good stuff.
Also very much like the idea of busting out each individual type of analysis into its own package, but also consider that there could/should be a similar set of methods the result of an analysis might want to provide, ie. a FacilePCA result would be a subclass of a FacileAnalysis, and these results might be required to provide a certain set of universal methods to better understand things like:
so that modules could interact with each other (which I'm not sure we have a great mechanism for), or with "the mother ship", which might be The FacileExplorer when these modules were run within the context of TFE as opposed to, perhaps, run individually from a user's workspace as a shiny gadget.
Yes, we need to write down how the modules and mother ship talk. The PCA module was the first example here, but from what I understand, it was way more complicated than what we want.
Pete
Peter M. Haverty, Ph.D. Genentech, Inc. phaverty@gene.com
On Fri, Sep 14, 2018 at 2:07 PM, Steve Lianoglou notifications@github.com wrote:
Yes, good stuff.
Also very much like the idea of busting out each individual type of analysis into its own package, but also consider that there could/should be a similar set of methods the result of an analysis might want to provide, ie. a FacilePCA result would be a subclass of a FacileAnalysis, and these results might be required to provide a certain set of universal methods to better understand things like:
- how they were run (ie. what "sample filter" was active, and the like).
- some set of things that other modules (or the FTE) could query itself
so that modules could interact with each other (which I'm not sure we have a great mechanism for), or with "the mother ship", which might be The FacileExplorer when these modules were run within the context of TFE as opposed to, perhaps, run individually from a user's workspace as a shiny gadget.
— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/Genentech/FacileDataSet/pull/19#issuecomment-421484663, or mute the thread https://github.com/notifications/unsubscribe-auth/AH02K-qqZ5engK51tz-hzV4hq9edxpcpks5ubBqngaJpZM4Wpdrr .
Updated documentation for organism, samples, fds, set_fds, assay_info_tbl, assay_sample_info_tbl, custom_sample_covariantes_tbl, feature_info_tbl, meta_file, meta_info, sample_covariantes_tbl, sample_info_tbl