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

Phytools removal #36

Merged
merged 3 commits into from Dec 14, 2019
Merged

Phytools removal #36

merged 3 commits into from Dec 14, 2019

Conversation

@taddallas
Copy link
Contributor

@taddallas taddallas commented Dec 12, 2019

Attempting to quickly address the risk of dependency hell, starting with cutting out the need for phytools and animation, as referenced in issue #34

I think phytools was only being used in the plotBeta function. Is this right? If not, then I need to hack away a bit more. I think it was only the untangle function though.

Build was successful on Ubuntu 18.04 machine using devtools::check()

@jarioksa
Copy link
Collaborator

@jarioksa jarioksa commented Dec 13, 2019

Travis checks failed, but this seems to be independent of this change: building the test environment (R etc) failed before starting the test for this PR. I checked this with the latest R-devel ( 2019-12-12 r77564 ) with no problem. However, I think this feature was not checked in those standard tests.

My reading of this PR against full phytools::untangle() is positive: it takes the subset of the original code that we actually need here. Some small comments:

  • We do not have to importFrom ape Ntip as it is not used in this code.
  • The PR unnecessarily changes 20 other files by re-formatting argument lists in their function definitions. While I think the new formatting is better (my editor does not like the over-long lines of R files in Hmsc), it confounds the essential change made here.
  • Check indentation in the embedded untangle: it is confusing to human readers.
@taddallas
Copy link
Contributor Author

@taddallas taddallas commented Dec 13, 2019

  • removed Ntip import. Good catch.
  • I just ran devtools::check() to run the package through the standard checks and tests. I guess this messed with the formatting of the man files? What's the reason behind this?
  • I think I've fixed the indentation, but perhaps not. I'm used to each tab being 2 spaces, and I preferentially use tabs over spaces, which may translate not well across text editors. Also, the three space indent takes some getting used to (but hopefully I've done it correctly now).
@jarioksa jarioksa merged commit ee7e26e into hmsc-r:master Dec 14, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taddallas taddallas deleted the taddallas:phytoolsRemoval branch Dec 15, 2019
@jarioksa jarioksa mentioned this pull request Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.