search

microbiome / mia

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

countDominantTaxa output table #387

Closed himmil closed 1 year ago

antagomir commented 1 year ago

We should use the same argument name than the round function itself, this will be more clear.

antagomir commented 1 year ago

up

antagomir commented 1 year ago

@himmil this PR has been open for a month; could we aim to close soon?

antagomir commented 1 year ago

This PR has been merged but it says above that "This branch is out-of-date with the base branch", and this PR also hasnt been closed. Can we do that as well, or is something else still missing?

himmil commented 1 year ago

Apparently the testing file needs to be edited as well once the output table has changed.

antagomir commented 1 year ago

Yes, update all relevant files and complete the PR. Ask for support through Slack where necessary.

antagomir commented 1 year ago

Resolve the conversation issues when they become solved. This way we can remove points that have been successfully addressed.

antagomir commented 1 year ago

This branch is out-of-date with the base branch

antagomir commented 1 year ago

Thanks - one more request.

Could you also deprecate the name countDominantTaxa and take into use new name countDominantFeatures?

You can check other deprecated functions for an example.

The unit tests and vignette should be updated accordingly, after merging also the OMA examples.

antagomir commented 1 year ago

check how to complete..

antagomir commented 1 year ago

still on the way?

himmil commented 1 year ago

I also added the changes regarding to issues #393 and #377 into this PR, apologies for the inconvenience..

antagomir commented 1 year ago

For clarity, perhaps you can announce when this PR is ready for review.

himmil commented 1 year ago

It's ready for review.

antagomir commented 1 year ago

ok - @TuomasBorman

antagomir commented 1 year ago

Hmm seems merging is blocked until @TuomasBorman approves too.

(we could see if it is possible to remove the requirement of multiple approvals)

himmil commented 1 year ago

I don't have write access so could you either merge it for me or give me the permission?