create new generic converter #506

thpralas · 2024-03-20T13:20:50Z

Create a new generic function convert to replace the following converters :

makeTreeSEFromPhyloseq
makeTreeSEFromBiom
makeTreeSEFromDADA2
makePhyloseqFromTreeSE

Merge branch 'draft_converters' of https://github.com/microbiome/mia into draft_converters # Conflicts: # R/convert.R

TuomasBorman

When dada2, phyloseq and biom ackages are not loaded, the functions says that these classes cannot be found --> I think that is ok. (just to note)

R/convert.R

…into draft_converters

Merge branch 'devel' of https://github.com/microbiome/mia into draft_converters # Conflicts: # NEWS

codecov · 2024-04-04T08:49:24Z

Codecov Report

Attention: Patch coverage is 53.39806% with 48 lines in your changes missing coverage. Please review.

Please upload report for BASE (devel@c17fa9f). Learn more about missing BASE report.

❗ Current head c5847e4 differs from pull request most recent head 2f906a1

Please upload reports for the commit 2f906a1 to get more accurate results.

Files	Patch %	Lines
R/deprecate.R	0.00%	25 Missing ⚠️
R/makephyloseqFromTreeSummarizedExperiment.R	78.18%	12 Missing ⚠️
R/convert.R	50.00%	11 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             devel     #506   +/-   ##
========================================
  Coverage         ?   72.73%           
========================================
  Files            ?       41           
  Lines            ?     4666           
  Branches         ?        0           
========================================
  Hits             ?     3394           
  Misses           ?     1272           
  Partials         ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

R/makeTreeSummarizedExperimentFromBiom.R

This reverts commit 167cb37.

…into draft_converters

Merge branch 'devel' of https://github.com/microbiome/mia into draft_converters # Conflicts: # NEWS # R/deprecate.R

thpralas · 2024-06-03T10:34:01Z

If you cannot find solution for this, we can also rename the existing functions and move the documentation under shared "convert" page.

makeTreeSEFromPhyloseq --> convertFromPhyloseq

makeTreeSEFromBiom --> convertFromBIOM

makeTreeSEFromDADA2 --> convertFromDADA2

makePhyloseqFromTreeSE --> convertToPhyloseq

Should I do that even though it's a note and not a warning? I'm still trying to understand the note better and determine if it's something to be concerned about.

TuomasBorman · 2024-06-03T12:35:26Z

This is what happens

> devtools::load_all()
ℹ Loading mia
in method for ‘convert’ with signature ‘x="dada"’: no definition for class “dada”
in method for ‘convert’ with signature ‘x="phyloseq"’: no definition for class “phyloseq”
in method for ‘convert’ with signature ‘x="biom"’: no definition for class “biom”
> library(dada2)
Loading required package: Rcpp
> devtools::load_all()
ℹ Loading mia
in method for ‘convert’ with signature ‘x="phyloseq"’: no definition for class “phyloseq”
in method for ‘convert’ with signature ‘x="biom"’: no definition for class “biom”

So it it caused, because dada2:dada class is not found. But when dada2 package is loaded, it is found. We do not want to add new dependencies, that is why those packages are not loaded when mia is loaded.

Previously these functions were implemented as "basic" functions, so they were not this kind of generic functions that choose method based on class.

In theory, you could probably add a script to mia.R that loads these packages if available, but that is suboptimal solution. I think it is not good that there are this kind of notes when mia is loaded. Some users might think that these are warnings.

TuomasBorman · 2024-06-04T14:07:42Z

Any thoughts @antagomir

antagomir · 2024-06-04T21:39:01Z

If we go with mia then I am OK with either solution. One generic function would be nice but if this is not possible, then specific functions.

Another solution could be a separate utility package. We might have other misc utility needs too. But that will be more maintenance overhead.

TuomasBorman · 2024-06-05T06:40:43Z

+ convert() is very neat solution
- We cannot have more dependencies, we have too many already
- We cannot have notes, when package is loaded. It does not look good.

--> I think we have to go with separate functions. You can just rename the functions from their original files and then just combine the documentation under convert

Merge branch 'devel' of https://github.com/microbiome/mia into draft_converters # Conflicts: # NEWS

…into draft_converters

TuomasBorman · 2024-06-13T07:41:35Z

NEWS

+  calculateCCA to getCCA
+ add informative error message in rarefyAssay on assays with strictly-negative values 


Problem with news, same line is added

TuomasBorman · 2024-06-13T07:54:27Z

R/makeTreeSummarizedExperimentFromPhyloseq.R

-    # input check
-    .require_package("phyloseq")
-    if(!is(obj,"phyloseq")){
-        stop("'obj' must be a 'phyloseq' object")
-    }
-    #
-    # Get the assay
-    counts <- obj@otu_table@.Data
-    # Check the orientation, and transpose if necessary
-    if( !obj@otu_table@taxa_are_rows ){
-        counts <- t(counts)
-    }
-    # Create a list of assays
-    assays <- SimpleList(counts = counts)
-
-    if(!is.null(obj@tax_table@.Data)){
-        rowData <- DataFrame(data.frame(obj@tax_table@.Data))
-    } else{
-        rowData <- S4Vectors::make_zero_col_DFrame(nrow(assays$counts))
-        rownames(rowData) <- rownames(assays$counts)
-    }
-    if(!is.null(obj@sam_data)){
-        colData <- DataFrame(data.frame(obj@sam_data))
-    } else{
-        colData <- S4Vectors::make_zero_col_DFrame(ncol(assays$counts))
-        rownames(colData) <- colnames(assays$counts)
-    }
-    if(!is.null(obj@phy_tree)){
-        rowTree <- obj@phy_tree
-    } else {
-        rowTree <- NULL
-    }
-    if (!is.null(obj@refseq)) {
-        referenceSeq <- obj@refseq
-    } else {
-        referenceSeq <- NULL
-    }
-    TreeSummarizedExperiment(assays = assays,
-                            rowData = rowData,


Not sure, if the link is just broken between files, but it seems now that you have copy.-pasted code to another file. Instead, it should only look like that the name has changed.

Also, as this is not anymore big change (only the name is changed and documentation is moved under common page), I am wondering if you should make new PR with new branch if that is easier.

We have parameter naming PR that will be soon merged, and it affects also these files. It was good that we explored the possibility of common convert function even though it will not be implemented.

I am wondering what is the easiest approach.

Yes I think it may be easier to open a new PR because I have probably made some unnecessary changes in this PR when I tried to implement convert generic function.

Should I make new methods for the functions that do not have one (e.g. makeTreeSEFromDADA2) ?

Also should we keep both names makeTreeSE* and makeTreeSummarizedExperiment* for every converters or not?

Now this bigger PR was merged #582, so you can continue with this.

Those names will be deprecated. All the functions that were available for user must be deprecated and added to deprecate.R file.

Each functionality will have only one function that is convertTo* or convertFrom*

Merge branch 'devel' of https://github.com/microbiome/mia into draft_converters # Conflicts: # NEWS # R/convertFromBIOM.R # R/makeTreeSummarizedExperimentFromPhyloseq.R # R/makephyloseqFromTreeSummarizedExperiment.R # man/makePhyloseqFromTreeSE.Rd # man/makeTreeSEFromBiom.Rd # man/makeTreeSEFromPhyloseq.Rd # tests/testthat/test-4IO.R # tests/testthat/test-5Unifrac.R

TuomasBorman · 2024-06-21T05:20:43Z

R/convertFromBIOM.R

        removeTaxaPrefixes = FALSE, rank.from.prefix = rankFromPrefix, 
        rankFromPrefix = FALSE,
        artifact.rm = remove.artifacts, remove.artifacts = FALSE, ...){
    # input check
    .require_package("biomformat")
-    if(!is(x,"biom")){


The name of object is changed from obj to x indicating that your branch is out of sync

TuomasBorman · 2024-06-21T05:22:56Z

R/convertToPhyloseq.R

+setGeneric("convertToPhyloseq", signature = c("x"),
+           function(x, ...)
+             standardGeneric("convertToPhyloseq"))
+
+#' @rdname convert
+#' @export
+setMethod("convertToPhyloseq",
+          signature = c(x = "SummarizedExperiment"),
+    function(x, assay.type = "counts", assay_name = NULL, ...){
+      # Input check
+      .require_package("phyloseq")
+      # Check that tse do not have zero rows
+      if(!all(dim(x) > 0)){
+        stop("'x' contains zero rows. 'x' can not be converted
+             to a phyloseq object.",
+             call. = FALSE)
+      }
+
+      if (!is.null(assay_name)) {
+        .Deprecated(old="assay_name", new="assay.type", "Now assay_name is deprecated. Use assay.type instead.")
+      }
+
+      # Check assay.type
+      .check_assay_present(assay.type, x)
+
+      # phyloseq object requires nonduplicated rownames. If there are 
+      # duplicated rownames, they are converted so that they are unique
+      if( any(duplicated(rownames(x))) ){
+        rownames(x) <- getTaxonomyLabels(x)
+      }
+      # List of arguments


These look like they are new functions even though they are not. The name has only changed. This cannot be merged since this breaks the history of the functions. One option is to create new branch and continue from there

OK I think I am going to create a new branch then, it will be easier

TuomasBorman · 2024-06-25T07:32:24Z

#591

thpralas added 16 commits March 18, 2024 12:25

new generic function convert

1bc1d14

keep the original functions in their original files

8d1f351

up

efeb240

up

fca1fbd

convert documentation

10d353b

up

2c27585

up

24366d7

correct documentation

a5898c6

correct internal functions naming

bb48891

up

7eb4616

Merge branch 'draft_converters' of https://github.com/microbiome/mia into draft_converters # Conflicts: # R/convert.R

up

ef58f1b

up

813f6e6

up

ecd88df

up

06b7dfa

up

6fcfcc0

correct convert man page

4a937fc

thpralas requested a review from TuomasBorman March 25, 2024 09:09

thpralas and others added 3 commits March 26, 2024 15:14

correct BiocCheck warnings

6afdc7e

up

3f31882

Merge branch 'devel' into draft_converters

5c57a91

TuomasBorman requested changes Apr 3, 2024

View reviewed changes

R/convert.R Outdated Show resolved Hide resolved

R/convert.R Outdated Show resolved Hide resolved

R/convert.R Outdated Show resolved Hide resolved

thpralas added 6 commits April 3, 2024 15:57

change param tree_name to tree.name for convert

580ad1b

up

2f7db75

Merge branch 'draft_converters' of https://github.com/microbiome/mia …

1f7cf4b

…into draft_converters

new parameter for choosing output type when converting SE and TSE

c8b6242

up

08277cb

merge devel and resolve conflicts

c5847e4

Merge branch 'devel' of https://github.com/microbiome/mia into draft_converters # Conflicts: # NEWS

up

ee4d240

TuomasBorman reviewed Apr 10, 2024

View reviewed changes

R/makeTreeSummarizedExperimentFromBiom.R Outdated Show resolved Hide resolved

thpralas added 3 commits June 3, 2024 10:33

Revert "fix lost braces"

70016dd

This reverts commit 167cb37.

Merge branch 'draft_converters' of https://github.com/microbiome/mia …

b977a1c

…into draft_converters

merge devel

332acfa

Merge branch 'devel' of https://github.com/microbiome/mia into draft_converters # Conflicts: # NEWS # R/deprecate.R

thpralas and others added 10 commits June 5, 2024 16:12

replace convert with new names in deprecate, biom and phyloseq

7129b67

merge devel

67db5f2

Merge branch 'devel' of https://github.com/microbiome/mia into draft_converters # Conflicts: # NEWS

replace convert with new functions

99ec6cd

rename converters files

f400ab4

rearrange convert man page

93afb81

Merge branch 'devel' into draft_converters

383adea

fix errors and warnings

da81e8d

Merge branch 'draft_converters' of https://github.com/microbiome/mia …

d360e0e

…into draft_converters

fix convertToPhyloseq

49a2a26

try to fix R CMD check note

e0590b8

TuomasBorman requested changes Jun 13, 2024

View reviewed changes

thpralas added 6 commits June 19, 2024 14:43

merge devel

9cc92a2

small fix

d236775

renaming files

fdbd8af

renaming files

a30c9fd

resolve R CMD check warning

2f906a1

TuomasBorman reviewed Jun 21, 2024

View reviewed changes

TuomasBorman requested changes Jun 21, 2024

View reviewed changes

TuomasBorman closed this Jun 25, 2024

TuomasBorman deleted the draft_converters branch June 25, 2024 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create new generic converter #506

create new generic converter #506

thpralas commented Mar 20, 2024

TuomasBorman left a comment

codecov bot commented Apr 4, 2024 •

edited

Loading

thpralas commented Jun 3, 2024

TuomasBorman commented Jun 3, 2024 •

edited

Loading

TuomasBorman commented Jun 4, 2024

antagomir commented Jun 4, 2024

TuomasBorman commented Jun 5, 2024

TuomasBorman Jun 13, 2024

TuomasBorman Jun 13, 2024

thpralas Jun 13, 2024

thpralas Jun 17, 2024

TuomasBorman Jun 18, 2024

TuomasBorman Jun 21, 2024

TuomasBorman Jun 21, 2024

thpralas Jun 24, 2024

TuomasBorman commented Jun 25, 2024

		calculateCCA to getCCA
		+ add informative error message in rarefyAssay on assays with strictly-negative values

create new generic converter #506

create new generic converter #506

Conversation

thpralas commented Mar 20, 2024

TuomasBorman left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 4, 2024 • edited Loading

Codecov Report

thpralas commented Jun 3, 2024

TuomasBorman commented Jun 3, 2024 • edited Loading

TuomasBorman commented Jun 4, 2024

antagomir commented Jun 4, 2024

TuomasBorman commented Jun 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TuomasBorman commented Jun 25, 2024

codecov bot commented Apr 4, 2024 •

edited

Loading

TuomasBorman commented Jun 3, 2024 •

edited

Loading