-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: fasta aligned action, and multi-sequence aamutation action #195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about this symbol != position.symbol_whose_bitmap_is_flipped
condition, otherwise this looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test this action in a unit test (now that we can have sequences of arbitrary length)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will maybe introduce those next week if it's okay. As we need a database for the query engine to run and don't have any such tests yet, we could maybe introduce a good framework for such tests, which I would do in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the fastaAligned_multiple.json endToEndTest already tests this function with sequences of differing lengths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the different edge and error cases in mind, e.g. the sequence name does not exist. And also testing that the resulting sequence is actually correct, since this is vital. The reconstruction of the sequence is relatively complex.
for (auto position_id = local.begin(); position_id != local.end(); position_id++) { | ||
const NucPosition& position = sequence_store.positions.at(position_id); | ||
for (const NUCLEOTIDE_SYMBOL symbol : NUC_SYMBOLS) { | ||
if (symbol != position.symbol_whose_bitmap_is_flipped && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it possible to not consider positions with flipped bitmaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed wrong if the flipped symbol is unequal to the reference sequence! Good catch!
"AminoAcidMutations action can have the field sequenceName of type string or an array of " | ||
"strings, but no other type" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"AminoAcidMutations action can have the field sequenceName of type string or an array of " | |
"strings, but no other type" | |
"The field sequenceName of AminoAcidMutations action must have type string or an array, if present." |
"AminoAcidMutations action can have the field sequenceName of type string or an array " | ||
"of " | ||
"strings, but no other type; while parsing array encountered the element " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"AminoAcidMutations action can have the field sequenceName of type string or an array " | |
"of " | |
"strings, but no other type; while parsing array encountered the element " + | |
"AminoAcidMutations action sequenceNames must be of type string, but found " + |
How final is the implementation of the |
Not final at all, but I just wanted to have something working, while writing it out to a buffer will still take some time I suppose. Maybe introduce a answer size limit for this action until we implement the compressed and/or streaming download |
Ok, for some working solution, this is definitely fine, although maybe not the solution we should build upon in LAPIS. |
…ap is different from the reference sequence symbol
This PR introduces an action to retrieve fasta sequences. (Either for one or multiple sequences)
And also includes a fix for AAMutation action to work for multiple sequences as it is a similar logic