-
Notifications
You must be signed in to change notification settings - Fork 182
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
Simplealign #46
base: master
Are you sure you want to change the base?
Simplealign #46
Conversation
…ve/master' into simplealign
Oops - comments from me not a student in my class...
|
I have send the update log to Chris Fields, who was my mentor during my coding project in Google summer of code. I think I can put the update log in the BioPerl wiki, so everybody can see it. If you pull the new SimpleAlign code, you will expect some failed tests from t/Align/SimpleAlign.t, you will see the explanation in the report. |
See the wiki page for details of the updates: |
Without objection I'll be reviewing and merging the 3 open github pull requests this week. |
Hi, Jay, I notice that there is no revision on the SimpleAlign code for the last few months. So this implementation should work without problem using the SimpleAlign2.t provided. Let me know if you need any clarification. Cheers, |
We have failing Travis-CI at the moment, so hold off on a merge. We're planning on a new point release of BioPerl 1.6.x in about 1 month, we might want to wait before merging this one in. |
Jun, after a long long wait I'm finally getting around to reviewing these. I've pulled them into a separate branch from master and will merge back in after checking tests. |
Hi, Chris, You will have some errors from the old t/Align/SimpleAlign.t. You can try t/Align/SimpleAlign2.t, which is edited to test the new Bio::SimpleAlign |
I saw the errors. Do you recall whether these are failing b/c the implementation details changed, or where the tests erroneous to begin with? |
These are mainly caused by different implementation of several methods. The changes are listed in the BioPerl wiki. |
Is there any appetite for me to rebase this and submit as a new PR? |
Thank you for looking into this PR. This project was done almost 14 years
ago. There may not be any real value in merging it into the main repo. But
let me know if you are still interested.
Jun
…On Sat, Nov 16, 2024 at 7:42 PM mohawk2 ***@***.***> wrote:
Is there any appetite for me to rebase this and submit as a new PR?
—
Reply to this email directly, view it on GitHub
<#46 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACDBPGUH5YA4LZGIMR55V32BAGBXAVCNFSM6AAAAABR5PLRXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOBQHEYTEMBTGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
@mohawk2 @yinjun111 despite the age it's still completely feasible to merge this in since tests exist. My recommendation would be to work off either a new branch or the original one, rebase to the current master, then make sure any changes are in place for at least some minimal supported version of perl. In addition, if the original SimpleAlign tests are breaking we need to consider renaming the newer module something else since it would be a breaking API change for the original SimpleAlign and anyone using it (which is stunningly quite a bit of code out there). That shouldn't be too hard, and we could in effect work towards deprecating the original module in place of this one. |
Hi, Chris,
This is my implementation of Bio::SimpleAlign. I have a detailed report describing the improvement to the code, and explaining the failed tests using t/Align/SimpleAlign.t. If you need any help, please just let me know.
Cheers,
Jun