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

Added a patch for extreme case (but important) bugs in the Rphylip code: #2

Merged

Conversation

bsennblad
Copy link

Checklist

  • [x ] Used a fork of the feedstock to propose changes
  • [x ] Bumped the build number (if the version is unchanged) (although the previous number for some reason was '1000'!! probably some reason for that but not the obvious choice)
  • [x ] Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)

The following bugs were fixed by applying a patch:

  1. Various read.xx file-reading functions failed due mis-implemented space-padding for the phylip format when number of taxa > 99 (i.e., the number has > 2 digits).
  2. Rconsense assumed at least one vertex in input trees and thus failed when input contained completely unresolved trees
    (3. As a bonus some previous fixes for debugging output in the original author's source code, but missing in the cran code was included.9

The bug in 2. was actually fixed by a patch in a previously applied PR to the riginal bioconda r-rphylip recipe. However, this seems to have been lost in later versions of the recipe, including the one migrated to conda-forge.

These bug fixes have also been submitted as PRs to the original author's Rphylip source repo, but I'm uncertain if this is checked any longer as nothing happens. I have sent a separate email to the original author (Liam Revell) asking him to address these PRs.

Notify maintainers
@MathiasHaudgaard
@johanneskoester
@bgruening
@daler
@jdblischak
(Note, maintainers FrodePedersen, ArneKr not found by github)

@conda-forge-admin, please rerender

Bengt Sennblad and others added 2 commits January 23, 2019 18:17
1) read.multi.xxf failed for infiles with >100 taxa due to erroneous assumptions on number of  prepending spaces before ntaxa statement,
2) rconsense failed for completely unresolved trees, and
3) as a bonus some verbosity fixes apparently not present in the cran version of the source got included).
A patch for the rconsense bug was previously merged to the original bioconda version of this recipe, but this seems to have been lost now(?!).
PRs for these changes has been submitted to the sorce Rphylip repo, but this seems not to be actively supported any longer:(.
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@bsennblad
Copy link
Author

@MathiasHaudgaard
@johanneskoester
@bgruening
@daler
@jdblischak
Hi,
Any comments? If not, could someone perhaps merge -- I don't have merging rights here myself.

THanks

Bengt

@jdblischak
Copy link
Member

@bsennblad Sorry for the delay. This is a complex PR.

(although the previous number for some reason was '1000'!! probably some reason for that but not the obvious choice)

The strange build number is a temporary hack during the compiler migration. You were correct to bump the build number to 1001.

The bug in 2. was actually fixed by a patch in a previously applied PR to the riginal bioconda r-rphylip recipe. However, this seems to have been lost in later versions of the recipe, including the one migrated to conda-forge.
These bug fixes have also been submitted as PRs to the original author's Rphylip source repo, but I'm uncertain if this is checked any longer as nothing happens.

It would make it a lot easier to review if you included links to the sources you are referencing (e.g. to the original bioconda recipe and the PR). I managed to find your PR, which was merged liamrevell/Rphylip#10

Since you have made such a good effort to update the original code (and the original author approved your changes), I'm going to merge this PR. Please continue to encourage the author to submit the updated version to CRAN. If that happens, then you can delete the patch.

Also, it would be nice if you could add a small file and then test that it can be read properly, but you can send that in a future PR if you like.

@jdblischak jdblischak merged commit 45a95af into conda-forge:master Feb 12, 2019
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.

None yet

4 participants