[MISC] Remove field::offset from sam_file_[in/out]put. #3089

smehringer · 2022-11-01T12:55:46Z

Follow up on #3058

vercel · 2022-11-01T12:55:50Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
seqan3	✅ Ready (Inspect)	Visit Preview	Nov 2, 2022 at 2:58PM (UTC)

eseiler · 2022-11-01T14:35:49Z

include/seqan3/io/sam_file/record.hpp

    {
-        return get_impl(field_constant<seqan3::field::offset>{}, static_cast<tuple_base_t &&>(*this));
+        return static_cast<int32_t>(0);


Suggested change

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.

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.alignment() 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

So in summary, I would remove the return completely, analog to alignment().

I agree, should just be

SEQAN3_DEPRECATED_340 decltype(auto) sequence_position() && {}

etc.

include/seqan3/io/sam_file/format_bam.hpp

eseiler · 2022-11-01T14:41:30Z

include/seqan3/io/sam_file/record.hpp

     *
     * 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() &&


In lieu of the other comment, would this work?

SEQAN3_DEPRECATED_340 int32_t sequence_position() && { return {}; }

…c_type_deduction

codecov · 2022-11-02T14:11:26Z

Codecov Report

Base: 98.23% // Head: 98.22% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (800ddc9) compared to base (92164e6).
Patch coverage: 85.71% of modified lines in pull request are covered.

❗ Current head 800ddc9 differs from pull request most recent head 8cc85e4. Consider uploading reports for the commit 8cc85e4 to get more accurate results

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

Impacted Files	Coverage Δ
include/seqan3/io/sam_file/format_sam.hpp	`95.09% <ø> (-0.08%)`	⬇️
include/seqan3/io/sam_file/input.hpp	`100.00% <ø> (ø)`
...nclude/seqan3/io/sam_file/input_format_concept.hpp	`100.00% <ø> (ø)`
include/seqan3/io/sam_file/output.hpp	`100.00% <ø> (ø)`
...clude/seqan3/io/sam_file/output_format_concept.hpp	`100.00% <ø> (ø)`
include/seqan3/io/sam_file/record.hpp	`100.00% <ø> (ø)`
...io/sam_file/simple_three_verbose_reads_fixture.hpp	`93.61% <ø> (-0.39%)`	⬇️
...lude/seqan3/io/sam_file/detail/format_sam_base.hpp	`97.81% <80.00%> (-0.04%)`	⬇️
include/seqan3/io/sam_file/format_bam.hpp	`94.92% <100.00%> (-0.08%)`	⬇️
...de/seqan3/argument_parser/detail/version_check.hpp	`92.96% <0.00%> (-0.79%)`	⬇️
... and 1 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

Co-authored-by: Enrico Seiler <[email protected]>

eseiler

LGTM, one 💅

I suggest we talk about the way we are "deprecating"/removing offset and alignment once this PR is merged.

Since we static_assert on 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

eseiler · 2022-11-04T13:37:44Z

include/seqan3/io/sam_file/format_bam.hpp

@@ -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);


Suggested change

int32_t const sc_front = soft_clipping_at_front(cigar_vector);

int32_t const sc_front{soft_clipping_at_front(cigar_vector)};

💅

eseiler · 2022-11-04T13:40:08Z

include/seqan3/io/sam_file/record.hpp

    {
-        return get_impl(field_constant<seqan3::field::offset>{}, static_cast<tuple_base_t &&>(*this));
+        return static_cast<int32_t>(0);


I agree, should just be

SEQAN3_DEPRECATED_340 decltype(auto) sequence_position() && {}

etc.

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Nov 1, 2022

eseiler reviewed Nov 1, 2022

View reviewed changes

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Nov 1, 2022

eseiler removed the lint [INTERNAL] signal for linting label Nov 2, 2022

smehringer and others added 5 commits November 2, 2022 14:41

[MISC] Remove field::offset from SAM/BAM files.

a1a3a23

[FIX] Remove offset from format_sam::read_alignment_record call

0a661dc

[FIX] Remove offset from sam_file_{input,output,record}_test

9e67d4d

[FIX] Remove offset from format_sam_benchmark

dbf45d9

[FIX] Remove offset from sam_file_input_construction_without_automati…

800ddc9

…c_type_deduction

eseiler force-pushed the io_remove_non_sam_fields branch from ef85b74 to 800ddc9 Compare November 2, 2022 13:42

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Nov 2, 2022

Update include/seqan3/io/sam_file/format_bam.hpp

8cc85e4

Co-authored-by: Enrico Seiler <[email protected]>

seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Nov 2, 2022

smehringer requested a review from eseiler November 4, 2022 11:05

eseiler approved these changes Nov 4, 2022

View reviewed changes

eseiler requested a review from SGSSGene November 4, 2022 13:43

SGSSGene approved these changes Nov 7, 2022

View reviewed changes

eseiler merged commit 8816830 into seqan:master Nov 7, 2022

smehringer deleted the io_remove_non_sam_fields branch November 17, 2022 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MISC] Remove field::offset from sam_file_[in/out]put. #3089

[MISC] Remove field::offset from sam_file_[in/out]put. #3089

smehringer commented Nov 1, 2022

vercel bot commented Nov 1, 2022 •

edited

Loading

eseiler Nov 1, 2022 •

edited

Loading

smehringer Nov 2, 2022 •

edited by eseiler

Loading

smehringer Nov 4, 2022

eseiler Nov 4, 2022

eseiler Nov 1, 2022

codecov bot commented Nov 2, 2022 •

edited

Loading

eseiler left a comment

eseiler Nov 4, 2022

eseiler Nov 4, 2022

	return static_cast<int32_t>(0);
	return format_sam_base::soft_clipping_at_front(cigar()); // Won't work as the base is protected

	int32_t const sc_front = soft_clipping_at_front(cigar_vector);
	int32_t const sc_front{soft_clipping_at_front(cigar_vector)};

[MISC] Remove field::offset from sam_file_[in/out]put. #3089

[MISC] Remove field::offset from sam_file_[in/out]put. #3089

Conversation

smehringer commented Nov 1, 2022

vercel bot commented Nov 1, 2022 • edited Loading

eseiler Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

smehringer Nov 2, 2022 • edited by eseiler Loading

Choose a reason for hiding this comment

smehringer Nov 4, 2022

Choose a reason for hiding this comment

eseiler Nov 4, 2022

Choose a reason for hiding this comment

eseiler Nov 1, 2022

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2022 • edited Loading

Codecov Report

eseiler left a comment

Choose a reason for hiding this comment

eseiler Nov 4, 2022

Choose a reason for hiding this comment

eseiler Nov 4, 2022

Choose a reason for hiding this comment

vercel bot commented Nov 1, 2022 •

edited

Loading

eseiler Nov 1, 2022 •

edited

Loading

smehringer Nov 2, 2022 •

edited by eseiler

Loading

codecov bot commented Nov 2, 2022 •

edited

Loading