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

S3 support for phyloseq and TreeSE #17

Merged
merged 12 commits into from
Aug 14, 2021
Merged

Conversation

antagomir
Copy link
Contributor

Opening this PR related to philr issue #16

All checks pass and this can be reviewed & necessary changes discussed but two things are pending before merge:

  • Confirmation that the mia package Bioc devel version has been updated (working on this) as some functionality for TreeSE examples is required from there
  • Links in the vignette (between sub-vignettes) may need an update, could not check fully yet

Kindly review and suggest any modifications.

@antagomir
Copy link
Contributor Author

This PR is now ready.

Summary of changes:

  • Added support for TreeSummarizedExperiment and phyloseq in the philr function via S3 classes

  • Revised the vignette to include examples with and without these data formats within a single workflow; we maintain the same workflow than previously; the independent phyloseq and miaverse frameworks can provide additional capabilities for tree handling, ordination, visualization etc. but I aimed to keep things simple and focused on philr functionality in this vignette.

  • Some minor changes added that were requested by Bioconductor checks

After merging, this should be bumped to Bioc devel.

Copy link
Owner

@jsilve24 jsilve24 left a comment

Choose a reason for hiding this comment

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

Thanks for putting all this together. I think this is a very nice contribution. Overall I just had two comments about adding required dependencies and about changing the default behavior of the philr function.

Happy to discuss more if you feel strongly on either.

R/philr.R Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
@antagomir
Copy link
Contributor Author

All requested changes implemented, with some bioc stilization. Happy to make further fixes if there is a need.

We could change philr input argument df to x on the same go if this is desired. Or leave as is.

Copy link
Owner

@jsilve24 jsilve24 left a comment

Choose a reason for hiding this comment

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

Thanks again for all this work. I think I found one error in the proposed pull -- defaults don't seem to have been changed in the main s3 function, only in the individual methods.
I think this will cause problems.

man/convert_to_long.Rd Show resolved Hide resolved
R/philr.R Outdated Show resolved Hide resolved
@antagomir
Copy link
Contributor Author

I think I have now made all requested changes but if not, let us continue discussion & fixing.

@jsilve24
Copy link
Owner

This looks great. Also I think you are correct, my appologies for the confusion regarding the default weights. Thank you for your patience and hard work. Merging now.

@jsilve24 jsilve24 merged commit 3d65124 into jsilve24:master Aug 14, 2021
@antagomir
Copy link
Contributor Author

Awesome, thanks - pleasure to contribute to such a great package! The phylofactor is possible next target.

I anticipate that the updates will be also pushed to Bioconductor devel upstream to facilitate further use: I noticed that running the standard check BiocCheck::BiocCheck() generates some more suggested coding style updates. But I expect that those are not mandatory for already accepted packages.

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.

2 participants