[MISC] Remove field::offset from sam_file_[in/out]put. #3089
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| { | ||
| return get_impl(field_constant<seqan3::field::offset>{}, static_cast<tuple_base_t &&>(*this)); | ||
| return static_cast<int32_t>(0); |
There was a problem hiding this comment.
| return static_cast<int32_t>(0); | |
| return format_sam_base::soft_clipping_at_front(cigar()); // Won't work as the base is protected |
Wouldn't we want something like this? Such that it is deprecated, but still does the correct thing.
Thinking about it, alignment() should probably throw instead of just doing nothing...though you cannot get to call alignment() because it will static assert.
I guess it's the same here :D
One could argue that it might help to provide the soft_clipping_at_front function. It does seem like a common use case. Especially since there seems to be a pitfall where there might be hard clipping in the first position and the soft clipping in the second.
There was a problem hiding this comment.
Wouldn't we want something like this? Such that it is deprecated, but still does the correct thing.
That was also my first impulse but it is not guaranteed that cigar is available. Maybe we should not return anything to mimic the alingment removal and avoid unexpected semantic issues.
Thinking about it, alignment() should probably throw instead of just doing nothing...though you cannot get to call alignment() because it will static assert.
Yes, you would never get to the exception because you would need to ignore the deprecation warning and then just use rec.alignme
8000
nt() without catching the result. When trying to catch it, you will get the error that alignment is void.
One could argue that it might help to provide the soft_clipping_at_front function. It does seem like a common use case. Especially since there seems to be a pitfall where there might be hard clipping in the first position and the soft clipping in the second.
Yes. But this would be part of several utility functions for a cigar vector.
We need to discuss them separately and should not implement them without a use case.
Utility that'd be nice:
- CIGAR String <-> CIGAR Vector transformation
- Get Hard/Softclipping at front or back
- Get Length of the reference sequence region
- Get Length of the query sequence region
There was a problem hiding this comment.
So in summary, I would remove the return completely, analog to alignment().
There was a problem hiding this comment.
I agree, should just be
SEQAN3_DEPRECATED_340 decltype(auto) sequence_position() &&
{}etc.
| * | ||
| * The position is the length of the soft-clipping at the start of the seqan3::sam_record::cigar_sequence if a | ||
| * soft-clipping is present and `0` otherwise. | ||
| */ | ||
| decltype(auto) sequence_position() && | ||
| SEQAN3_DEPRECATED_340 decltype(auto) sequence_position() && |
There was a problem hiding this comment.
In lieu of the other comment, would this work?
SEQAN3_DEPRECATED_340 int32_t sequence_position() &&
{
return {};
}ef85b74 to
800ddc9
Compare
Codecov ReportBase: 98.23% // Head: 98.22% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3089 +/- ##
==========================================
- Coverage 98.23% 98.22% -0.02%
==========================================
Files 275 275
Lines 12197 12175 -22
==========================================
- Hits 11982 11959 -23
- Misses 215 216 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Co-aut 8000 hored-by: Enrico Seiler <eseiler@users.noreply.github.com>
There was a problem hiding this comment.
LGTM, one 💅
I suggest we talk about the way we are "deprecating"/removing offset and alignment once this PR is merged.
- Since we
static_asserton the fields, we cannot even call the member functions on the record (?) - To deprecate something usually means that it still works, but will be removed later. Which is not the case here.
- We can still have the record member functions for documentation purposes, i.e. wrap the members in a
#if SEQAN3_DOXYGEN_ONLY(1)0
| @@ -515,16 +505,18 @@ format_bam::read_alignment_record(stream_type & stream, | |||
| // ------------------------------------------------------------------------------------------------------------- | |||
| if constexpr (!detail::decays_to_ignore_v<cigar_type>) | |||
| { | |||
| int32_t const sc_front = soft_clipping_at_front(cigar_vector); | |||
There was a problem hiding this comment.
| int32_t const sc_front = soft_clipping_at_front(cigar_vector); | |
| int32_t const sc_front{soft_clipping_at_front(cigar_vector)}; |
💅
| { | ||
| return get_impl(field_constant<seqan3::field::offset>{}, static_cast<tuple_base_t &&>(*this)); | ||
| return static_cast<int32_t>(0); |
There was a problem hiding this comment.
I agree, should just be
SEQAN3_DEPRECATED_340 decltype(auto) sequence_position() &&
{}etc.
Follow up on #3058