search

phenoscape / rphenoscape

R package to make phenotypic traits from the Phenoscape Knowledgebase available from within R.
https://rphenoscape.phenoscape.org/
Other
5 stars 5 forks source link

Adds mutual exclusion function #238

Closed johnbradley closed 2 years ago

johnbradley commented 2 years ago

Adds mutually_exclusive() function to determine whether two or more phenotypes are mutually exclusive. Based on https://github.com/arathee2/rphenoscape/tree/mutual-exclusivity.

Fixes #237

johnbradley commented 2 years ago

I updated the deprecated functions in the mutual-exclusion vignette.

The source code of the original two functions (mutual_exclusivity_pair() and mutual_exclusivity()) was embedded in the mutual-exclusion vignette and goes along with the discussion there.

hlapp commented 2 years ago

@johnbradley did you mean to take this PR out of draft mode?

johnbradley commented 2 years ago

@johnbradley did you mean to take this PR out of draft mode?

@hlapp - No. I don't think this is ready for merging yet. It needs your input on what changes are needed for the vignette. I also want to rebase these commits into a single commit before merging (unless you have some objection to doing so). I left the commits separate right now so it's easier to distinguish the individual changes so far.

johnbradley commented 2 years ago

With the latest change mutually_exclusive() is down to 13 minutes to process all the phenotypes from the Dillman et al. (2016) study. 5 minutes are spent running as.phenotype(..., withTaxa=TRUE). The progress bar is really flickery in RStudio now.

hlapp commented 2 years ago

Very nice! And that almost half of the time is now spent on API queries is more like how it should be (i.e., that executing necessary API queries is most or at least a large fraction of the execution time).

johnbradley commented 2 years ago

I made some comments and suggestions earlier, but it doesn't seem you've responded to any of those yet?

@hlapp What comments/suggestions are you referring to? I don't see any earlier comments/suggestions on this PR that haven't already been addressed.

hlapp commented 2 years ago

They're no longer showing as pending now, presumably you committed them.

Looks like the only remaining question is what to do with the vignette. It seems odd (and unhelpful) to repeat definitions of the functions there. I'm torn between retiring the vignette altogether, having the examples section show how to use the function, and having some more extended version of examples in the vignette.