accept other than ranks in addDominant

[Daenarys8]

ping: #415

[antagomir]

When the functionalitys is modified/extended, it is good to do the following by default:

• include relevant new unit tests
• update the manpage documentation

Is there something we could have on that?

[Daenarys8]

good to check

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@           Coverage Diff            @@
##             devel     #623   +/-   ##
========================================
  Coverage         ?   67.95%           
========================================
  Files            ?       43           
  Lines            ?     5249           
  Branches         ?        0           
========================================
  Hits             ?     3567           
  Misses           ?     1682           
  Partials         ?        0           

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

[antagomir]

Confirm when this is ready.

[Daenarys8]

Confirm when this is ready.

It is ready. Waiting on @TuomasBorman review.

[antagomir]

The documentations explains now: "With \code{rank} parameter, it is possible to agglomerate taxa based on taxonomic
ranks. E.g. if 'Genus' rank is used, all abundances of same Genus are added
together, and those families are returned. See \code{agglomerateByRank()} for
additional arguments to deal with missing values or special characters."

It might be helpful to also mention that in addition to "rank", other grouping variables would be accepted.

In fact, "rank" is somewhat misleading argument name if any grouping variable is accepted. Should we consider renaming this argument (as it is not only about ranks)?

[TuomasBorman]

The documentations explains now: "With \code{rank} parameter, it is possible to agglomerate taxa based on taxonomic ranks. E.g. if 'Genus' rank is used, all abundances of same Genus are added together, and those families are returned. See \code{agglomerateByRank()} for additional arguments to deal with missing values or special characters."

It might be helpful to also mention that in addition to "rank", other grouping variables would be accepted.

In fact, "rank" is somewhat misleading argument name if any grouping variable is accepted. Should we consider renaming this argument (as it is not only about ranks)?

At least the documentation should clearly mention what the function does. rank might still be good parameter name if the main purpose of the function is to agglomerate the data to taxonomy level and other groupings are rarely used

[antagomir]

At least the documentation should clearly mention what the function does. rank might still be good parameter name if the main purpose of the function is to agglomerate the data to taxonomy level and other groupings are rarely used

• agglomerateByVariable() is using argument name "f";
• agglomerateByRank() is using argument name "rank"

Seems to me that here the argument name "f" would be closer to the intended purpose, although it is not very clear as an argument name.

[TuomasBorman]

I believe the f comes from base functions where f is used to define grouping factors https://www.rdocumentation.org/packages/base/versions/3.6.2/topics/split

I do not have strict opinion on this. However, I think we should either use f or rank or then we should modify all f arguments in mia (maybe group). There are advantages when we use same argument names wherever possible.

[Daenarys8]

Hmm, so are we going with group as the argument in addDominant?

[antagomir]

group would be more clear than f but the latter (f) has advantage that it could be in many cases a passed variable and thus add less (?) maintenance. I agree that we should then aim to harmonize this across all functions.

[Daenarys8]

should i amend changes in new PR or current PR? @TuomasBorman

[TuomasBorman]

I think we can merge this and implement the other changes in other PR. scuttle package uses f as we currently. Of course, we can make our own decisions.

Also, because we have kind of changed the meaning of f, we should change the argument name (usually f is vector, but in mia it can be also a name of grouping variable).

However, I think it is little bit problematic if getDominant() does different things even thought the parameters are the "same". I am referring here that the agglomeration is done differently if the grouping variable is taxonomy rank or not. The problem is that this is not transparent for user.

# Uses agglomerateByRank
getDominant(tse, group = "Genus")
# Uses agglomerateByVariable
getDominant(tse, group = "bacteria_group") 

At least, we should notice this problem and document this as clearly as possible.

1. Change the argument name f -> group everywhere in mia
2. Change rank parameter in getDominant to group.

[Daenarys8]

I think this good. I have opened new issue for suggested changes.