search

microbiome / mia

Microbiome analysis
https://microbiome.github.io/mia/
Artistic License 2.0
45 stars 25 forks source link

Organize parameter names v2 #582

Closed Daenarys8 closed 2 weeks ago

Daenarys8 commented 3 weeks ago

Ping #579 .

TuomasBorman commented 3 weeks ago

Before I start review, is this duplicated with https://github.com/microbiome/mia/pull/562? What is the difference?

TuomasBorman commented 3 weeks ago

With quick look, everything seems ok, Can you still run BiocChec::BiocCheck() and ensure that lines that are modified in this PR follows Bioconductor guidelines? (If you did not do that already)

I will check this PR as soon as possible

Daenarys8 commented 3 weeks ago

BiocChec::BiocCheck() has no errors and three warnings. This PR re-did #562 because the latter's history was broken after rebase.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 81.02345% with 89 lines in your changes missing coverage. Please review.

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

:exclamation: Current head a22bd03 differs from pull request most recent head f01097f

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

Files Patch % Lines
R/importMetaphlan.R 0.00% 13 Missing :warning:
R/getCrossAssociation.R 89.58% 10 Missing :warning:
R/importMothur.R 66.66% 8 Missing :warning:
R/importQIIME2.R 71.42% 8 Missing :warning:
R/calculateUnifrac.R 73.07% 7 Missing :warning:
R/makeTreeSummarizedExperimentFromBiom.R 70.58% 5 Missing :warning:
R/merge.R 44.44% 5 Missing :warning:
R/taxonomy.R 76.19% 5 Missing :warning:
R/getPrevalence.R 83.33% 4 Missing :warning:
R/makeTreeSummarizedExperimentFromPhyloseq.R 71.42% 4 Missing :warning:
... and 12 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## devel #582 +/- ## ======================================== Coverage ? 67.37% ======================================== Files ? 41 Lines ? 4928 Branches ? 0 ======================================== Hits ? 3320 Misses ? 1608 Partials ? 0 ```

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

TuomasBorman commented 3 weeks ago

BiocChec::BiocCheck() has no errors and three warnings. This PR re-did #562 because the latter's history was broken after rebase.

Thanks!

TuomasBorman commented 2 weeks ago

MARGIN issue will be fixed in different PR https://github.com/microbiome/mia/issues/578

TuomasBorman commented 2 weeks ago

Let's wait Leo's thoughts

antagomir commented 2 weeks ago

I might be missing something, is there a specific question or is this about general code review?

TuomasBorman commented 2 weeks ago

General code review. This is affecting the whole system so good to have second reviewer

antagomir commented 2 weeks ago

done

TuomasBorman commented 2 weeks ago

BiocChec::BiocCheck() has no errors and three warnings. This PR re-did #562 because the latter's history was broken after rebase.

ABout those three warnings, is it possible to fix those?

Daenarys8 commented 2 weeks ago

There were three warnings. 1- WARNING: .Deprecated / .Defunct usage (found 76 times) 2- empty or missing \value sections found in man pages 3- empty or missing \format sections found in data man pages

1- requires that when deprecating functions we follow a three cycle process where we deprecate, defunct and then remove..

2- is caused by deprecate.Rd since it has no @return value.

  1. is caused by mia-datasets.Rd because it has no @format.

2 and 3 are easily handled but 1. seems to be a bit complicated.

TuomasBorman commented 2 weeks ago

There were three warnings. 1- WARNING: .Deprecated / .Defunct usage (found 76 times) 2- empty or missing \value sections found in man pages 3- empty or missing \format sections found in data man pages

1- requires that when deprecating functions we follow a three cycle process where we deprecate, defunct and then remove..

2- is caused by deprecate.Rd since it has no @return value. 3. is caused by mia-datasets.Rd because it has no @format.

2 and 3 are easily handled but 1. seems to be a bit complicated.

As they are not directly related to this PR, warnings are not fixed in this PR

TuomasBorman commented 2 weeks ago

Looks good. I will take a second look with fresh eyes, but I think this PR is finished

TuomasBorman commented 2 weeks ago

miaViz and OMA should be updated

Daenarys8 commented 2 weeks ago

Probably miaViz should precede OMA as OMA is dependable

TuomasBorman commented 2 weeks ago

Maybe, miaViz is faster to do. However, as we did not remove the support for old parameter names, there is no rush; the old names are still working

antagomir commented 2 weeks ago

Can you also check miaTime and miaSim R/, man/, tests/ and vignettes/