create new generic converter

[thpralas]

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

• makeTreeSEFromPhyloseq
• makeTreeSEFromBiom
• makeTreeSEFromDADA2
• makePhyloseqFromTreeSE

[codecov[bot]]

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.

:exclamation: 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 :warning:
R/makephyloseqFromTreeSummarizedExperiment.R	78.18%	12 Missing :warning:
R/convert.R	50.00%	11 Missing :warning:

Additional details and impacted files

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[TuomasBorman]

With these, I think good to go

[TuomasBorman]

Works nicely. @antagomir

[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)

I am still wondering this. It is not good that these warning messages occur when the package is loaded. Can you try to figure out fix for this?

One possibility is to add packages to dependencies but we don't want to do that. Other option is to make non-generic fuctions (like convertPhyloseq and convertToPhyloseq), but this generic function is much more tidier solution

[thpralas]

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)

I am still wondering this. It is not good that these warning messages occur when the package is loaded. Can you try to figure out fix for this?

One possibility is to add packages to dependencies but we don't want to do that. Other option is to make non-generic fuctions (like convertPhyloseq and convertToPhyloseq), but this generic function is much more tidier solution

I added import tags for these classes above each method and the namespace what modified accordingly. I think the warning messages do not occur anymore when loading the package

[thpralas]

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)

I am still wondering this. It is not good that these warning messages occur when the package is loaded. Can you try to figure out fix for this? One possibility is to add packages to dependencies but we don't want to do that. Other option is to make non-generic fuctions (like convertPhyloseq and convertToPhyloseq), but this generic function is much more tidier solution

I added import tags for these classes above each method and the namespace what modified accordingly. I think the warning messages do not occur anymore when loading the package

Actually this creates dependencies errors with R CMD check, I am going to look for another solution

[TuomasBorman]

https://github.com/microbiome/OMA/pull/484

[TuomasBorman]

Any updates on this? Should we go with generic function or specific functions for each type

convertFromPhyloseq/convertToPhyloseq

[antagomir]

I like the idea of generic function if it can be reasonably implemented.

It would be good to get this case closed soon in any case.

[TuomasBorman]

It works nicely but the problem is that the package loading gives warning. This is caused because the class in generic. For example phyloseq class is not detected if phyloseq package is not loaded, and mia does not load it by default.

[antagomir]

Ah right.. is there any way to deal with this automagically somehow..

[thpralas]

I have removed the import tags and it seems like the warnings do not appear anymore. However, there is a new R CMD check note : Warning: <anonymous>: ... may be used in an incorrect context: ‘convert(...)’ I am going to look into this and try to fix it.

[antagomir]

Might be that "convert" is too generic name, reserved for other purposes?

[TuomasBorman]

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

[thpralas]

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]

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]

Any thoughts @antagomir

[antagomir]

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]

+ 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

[TuomasBorman]

https://github.com/microbiome/mia/pull/591