-
Notifications
You must be signed in to change notification settings - Fork 13
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
Rehaul BAM and SAM accessor functions #28
base: develop
Are you sure you want to change the base?
Conversation
Changed printing of records:
After, empty record
Before, filled record:
After, filled record:
|
function quality_string(quals::Vector{UInt8}) | ||
characters = Vector{Char}(undef, length(quals)) | ||
for i in eachindex(quals) | ||
@inbounds qual = quals[i] | ||
if qual < 10 | ||
char = ' ' | ||
elseif qual < 15 | ||
char = '▁' | ||
elseif qual < 20 | ||
char = '▂' | ||
elseif qual < 25 | ||
char = '▃' | ||
elseif qual < 30 | ||
char = '▄' | ||
elseif qual < 35 | ||
char = '▆' | ||
elseif qual < 40 | ||
char = '▇' | ||
elseif qual < 255 | ||
char = '█' | ||
else | ||
char = '?' | ||
end | ||
@inbounds characters[i] = char | ||
end | ||
return join(characters) | ||
end |
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.
What about factoring out the kernel?
function quality_char(qual::UInt8)
if qual < 10
return ' '
end
if qual < 15
return '▁'
end
if qual < 20
return '▂'
end
if qual < 25
return '▃'
end
if qual < 30
return '▄'
end
if qual < 35
return '▆'
end
if qual < 40
return '▇'
end
if qual < 255
return '█'
end
return '?'
end
function quality_string(quals::Vector{UInt8})
return String(quality_char.(quals))
end
I think the has and is functions that provide an interpretation of the data/record are useful; the clear case being the flag field. However, the has functions are not necessary for checking the presence of a value in a mandatory field. Would it be accurate to say that we're attempting to balance out the issue of consistency between SAM and BAM: whether it is useful to have consistency at the data-level or whether it is sufficient to have consistency at the print-level? In any case, I generally think accessors should assume the presence of value. Also, once the raw byte data of a record has been interpreted, I'm not wedded to the idea that the record must maintain an exact byte structure of the original.
I would now lean towards nothing too -- the use of missing was annoying when chaining field comparisons. BTW, I like that base quality print out. |
I think it's useful to think in terms of three levels:
The issue with the API level is that there may be a conflict between having an easy API and having a performant one. In some circumstances it may be too difficult to NOT expose some of the underlying data, even though it shouldnt be relevant at the API level. Actually I think the best thing may be to just make a new branch and play around a bit with it. Then I'll try to use the package in some small test code and get a feel for it. |
You present the levels well. I wonder whether we gain anything by not locking ourselves into the spec at the data-level? To elaborate, there are the raw data files as per BAM and SAM specs, and our interpretation of the raw data which we keep in our struct. We could translate the raw data (null defaults) as they are loaded into our struct and then again if they are written to file. If we can rule out translation at that level, then the remaining spot for translation is in the accessors and setters. |
I don't think there is anything to gain by pushing the translation to the bottom layer. It increases complexity and introduces a performance penalty in that the fields would be translated unnecessarily for records that after a primary interrogation of the flag field would be discarded. It makes sense to defer translation to if and when you require the field data. I'm not particularly bothered by the possibility of repeated translation with multiple field access, as these fields in my experience are single-use, wouldn't you agree @jakobnissen? |
Yes, I agree. There's also something rather nice about having the layout of a BAM struct be exactly equal to its representation in a file. |
See also #24 and #25 .
This is a work in progress. In particular, I will first change the BAM code and get that right, and then afterwards change the SAM code to behave identically. Ideally, I would like SAM and BAM records to behave almost exactly identical, but that might not be feasible, let's see.
Changes: (updated as I go along):
hasposition
to always return a Boolisfilled
andhasflag
has been removedThings to mull over
has
-functions, or just return a specific sentinel value when the record doesn't have the information available, e.g.nothing
? The latter may be neater.nothing
(because it so qucikly errors if you dont handle it correctly, that leads to fewer bugs for the end user), but if you wantmissing
, that's fine as well.