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

Simplealign #46

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Simplealign #46

wants to merge 9 commits into from

Conversation

yinjun111
Copy link

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

@hyphaltip
Copy link
Member

Oops - comments from me not a student in my class...

So this currently fails in the travis build -- can someone comment about what it will take to fix it.
Where is the detailed report on what was added Jun - is it in the code or in a mail to the mailing list?

@yinjun111
Copy link
Author

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.

@yinjun111
Copy link
Author

See the wiki page for details of the updates:
http://www.bioperl.org/wiki/SimpleAlign_Updates#Abstract

@jhannah
Copy link
Member

jhannah commented Feb 5, 2013

Without objection I'll be reviewing and merging the 3 open github pull requests this week.

@yinjun111
Copy link
Author

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,
Jun

@cjfields
Copy link
Member

cjfields commented Feb 9, 2013

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.

@cjfields
Copy link
Member

cjfields commented Aug 9, 2013

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.

@yinjun111
Copy link
Author

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

@cjfields
Copy link
Member

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?

@yinjun111
Copy link
Author

These are mainly caused by different implementation of several methods. The changes are listed in the BioPerl wiki.
http://www.bioperl.org/wiki/SimpleAlign_Updates#Abstract

@mohawk2
Copy link

mohawk2 commented Nov 17, 2024

Is there any appetite for me to rebase this and submit as a new PR?

@yinjun111
Copy link
Author

yinjun111 commented Nov 17, 2024 via email

@cjfields
Copy link
Member

@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.

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.

5 participants