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

works with sequences sharing same ids #119

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

Conversation

FredericBGA
Copy link
Contributor

@FredericBGA FredericBGA commented Oct 18, 2019

a need that pop-up today from one of my colleague.

@peterjc
Copy link
Owner

peterjc commented Oct 18, 2019

Just looking at the code I don't understand the goal here - do you have a simplified example with the before/after behaviour?

@FredericBGA
Copy link
Contributor Author

FredericBGA commented Oct 21, 2019

Of course.

I had updated the tests accordingly.

before this PR, with this kind of file:

>7
ATGC
>7
ATGC

the result:

>7;7 representing 2 records
ATGC
>7;7 representing 2 records
ATGC

now the result file is:

>7;7 representing 2 records
ATGC

We often have this case, as we fetch sequences from various sources or people and try to merge them.

Is this clear for you?

@FredericBGA
Copy link
Contributor Author

The failed tests are not related to this PR, right?

@peterjc
Copy link
Owner

peterjc commented Oct 21, 2019

This is input query files with repeated identifiers?

Repeated entries with the same identifier and sequence are one thing, repeated identifiers with different sequence are another. Personally I would make these an error condition - they cause too many problems downstream.

@peterjc
Copy link
Owner

peterjc commented Oct 21, 2019

The tool's master branch is failing on TravisCI against the Galaxy dev branch, see #120

@FredericBGA
Copy link
Contributor Author

you're right, the output is not yet perfect in this case:

>1
A
>1
A
>1
T

the output was:

>1;1 representing 2 records
A
>1;1 representing 2 records
A
>1;1 representing 2 records
T

with this PR it's now (and not good either...):

>1;1 representing 2 records
A

@FredericBGA
Copy link
Contributor Author

@peterjc This new version works for what I was needed. I let you review the code and merge if you want. Thank you for your comments.

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