search

microbiome / mia

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

Organize parameter names #562

Closed Daenarys8 closed 3 weeks ago

Daenarys8 commented 1 month ago

This commit organizes parameters used by user.

TuomasBorman commented 1 month ago

Should we use MARGIN as a parameter name for instance in agglomerateByVariable https://github.com/microbiome/mia/pull/556

TuomasBorman commented 1 month ago

The fail of check is not related to this PR

TuomasBorman commented 1 month ago

Does this already take into account all parameters from the table?

Daenarys8 commented 1 month ago

Does this already take into account all parameters from the table?

yes. Except for obj parameter in summary. For some reason it rejects and throws error about signature.

TuomasBorman commented 1 month ago

Btw use normal push instead of force push for common push operation.

This might be hazardous if someone else is also working in this PR. You are forcing the history be the same as in your local. Force push should not be the ordinary tool to use

TuomasBorman commented 1 month ago

Does this already take into account all parameters from the table?

yes. Except for obj parameter in summary. For some reason it rejects and throws error about signature.

You should change also the signature signature = c(object = "SummarizedExperiment")--> signature = c(x = "SummarizedExperiment")

Daenarys8 commented 1 month ago

Does this already take into account all parameters from the table?

yes. Except for obj parameter in summary. For some reason it rejects and throws error about signature.

You should change also the signature signature = c(object = "SummarizedExperiment")--> signature = c(x = "SummarizedExperiment")

I did change the signature. Then it issued a warning about :

Warning message:
multiple methods tables found for ‘summary’
TuomasBorman commented 1 month ago

Ahh, it seems that generic summary function comes from R ecosystem. We have not defined summary as generic function, we have only defined methods for summary function. That R ecosystem summary uses "object" in its signature. So this problem comes from there.

We can keep the argument name as object in this case

TuomasBorman commented 1 month ago

@antagomir Thoughts? I think the naming convention is now good with these changes.

MARGIN is the only naming-thing left https://github.com/microbiome/mia/pull/556

codecov[bot] commented 4 weeks ago

Codecov Report

Attention: Patch coverage is 80.94218% 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 ed597c4 differs from pull request most recent head 87a981a

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

Files Patch % Lines
R/getCrossAssociation.R 89.58% 10 Missing :warning:
R/importMetaphlan.R 0.00% 10 Missing :warning:
R/calculateUnifrac.R 63.63% 8 Missing :warning:
R/importMothur.R 70.37% 8 Missing :warning:
R/importQIIME2.R 69.23% 8 Missing :warning:
R/merge.R 44.44% 5 Missing :warning:
R/agglomerate.R 73.33% 4 Missing :warning:
R/makeTreeSummarizedExperimentFromBiom.R 71.42% 4 Missing :warning:
R/makeTreeSummarizedExperimentFromPhyloseq.R 71.42% 4 Missing :warning:
R/taxonomy.R 78.94% 4 Missing :warning:
... and 13 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## devel #562 +/- ## ======================================== Coverage ? 67.58% ======================================== Files ? 41 Lines ? 4992 Branches ? 0 ======================================== Hits ? 3374 Misses ? 1618 Partials ? 0 ```

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

TuomasBorman commented 3 weeks ago

Closing since duplicated with https://github.com/microbiome/mia/pull/582