Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

countDominantTaxa output table #387

Merged
merged 30 commits into from
Aug 30, 2023
Merged

Conversation

himmil
Copy link
Contributor

@himmil himmil commented Jul 6, 2023

No description provided.

R/summaries.R Outdated Show resolved Hide resolved
Copy link
Contributor

@TuomasBorman TuomasBorman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

round could be an integer --> that would simplify the code

How about using digits as parameter name? round() has a parameter called that.

Also add input check for round parameter

@antagomir
Copy link
Member

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

R/summaries.R Outdated Show resolved Hide resolved
R/summaries.R Outdated Show resolved Hide resolved
R/summaries.R Outdated Show resolved Hide resolved
R/summaries.R Outdated Show resolved Hide resolved
@antagomir
Copy link
Member

up

@antagomir
Copy link
Member

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

@antagomir
Copy link
Member

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
Copy link
Contributor Author

himmil commented Aug 8, 2023

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

@antagomir
Copy link
Member

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

@antagomir
Copy link
Member

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

@antagomir
Copy link
Member

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

@antagomir
Copy link
Member

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
Copy link
Member

check how to complete..

Copy link
Contributor

@TuomasBorman TuomasBorman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, check comments; just minor things

R/summaries.R Outdated Show resolved Hide resolved
R/summaries.R Outdated Show resolved Hide resolved
R/summaries.R Outdated Show resolved Hide resolved
@antagomir
Copy link
Member

still on the way?

@himmil
Copy link
Contributor Author

himmil commented Aug 22, 2023

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

@antagomir
Copy link
Member

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

@himmil
Copy link
Contributor Author

himmil commented Aug 22, 2023

It's ready for review.

@antagomir
Copy link
Member

ok - @TuomasBorman

R/summaries.R Outdated Show resolved Hide resolved
R/summaries.R Outdated Show resolved Hide resolved
R/summaries.R Outdated Show resolved Hide resolved
R/summaries.R Show resolved Hide resolved
R/summaries.R Outdated Show resolved Hide resolved
@antagomir
Copy link
Member

Hmm seems merging is blocked until @TuomasBorman approves too.

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

@himmil
Copy link
Contributor Author

himmil commented Aug 30, 2023

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

@TuomasBorman TuomasBorman merged commit 046d679 into microbiome:master Aug 30, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants