[I/O] Remove column interface for structure files

[joergi-w]

Resolves #1397.

[codecov]

Codecov Report

Merging #1398 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1398      +/-   ##
==========================================
- Coverage   97.55%   97.53%   -0.02%     
==========================================
  Files         227      227              
  Lines        8785     8726      -59     
==========================================
- Hits         8570     8511      -59     
  Misses        215      215

Impacted Files	Coverage Δ
include/seqan3/io/structure_file/output.hpp	100% <ø> (ø)	⬆️
include/seqan3/io/structure_file/input.hpp	100% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfcc96e...b6e51bd. Read the comment docs.

[Irallia]

Looks good!
It is difficult to review deletions.
I have done my best to compare the deletions with the points given in #1397 and browsed the existing files, that nothing is left.

[joergi-w]

Thank you!

[smehringer]

I think the output must not be changed! Only the get interface of input files because it is broken. @seqan/core am I right? Sorry if I did not make this clear in the task! :( But I see that you have separate commits 👍 So it will be easy to revert it

With the code you removed you know cannot do fout = fin anymore can you? Is this tested? (It is for sequence files)

Also: Please add a notion of this change in the changelog! since it is an API change :)

[joergi-w]

With the code you removed you know cannot do fout = fin anymore can you? Is this tested? (It is for sequence files)

Yes, fout = fin is still possible and tested here:

seqan3/test/unit/io/structure_file/structure_file_output_test.cpp

Lines 378 to 380 in cfcc96e

	structure_file_input fin{std::istringstream{inp}, format_vienna{},
	fields<field::SEQ, field::ID, field::STRUCTURE>{}};
	assign_impl(fin);

seqan3/test/unit/io/structure_file/structure_file_output_test.cpp

Lines 324 to 331 in cfcc96e

	template<typename source_t>
	void assign_impl(source_t && source)
	{
	structure_file_output fout{std::ostringstream{}, format_vienna{}};
	fout = source;
	fout.get_stream().flush();
	EXPECT_EQ(reinterpret_cast<std::ostringstream&>(fout.get_stream()).str(), output_comp);
	}

[rrahn]

I think the output must not be changed! Only the get interface of input files because it is broken. @seqan/core am I right?

Well, the output files don't have a get interface so that shouldn't be a problem.
However, I am not completely convinced that the additional tuple/record based interface adds a lot of value. I am certainly not convinced that we have a huge performance gain here, because we save one pointer indirection when writing the records. So I wouldn't mind removing it as well?

I mean it would essentially boil down to the decision of whether 1 is so much better than 2.

/*1*/ fout = std::tie(c1, c2, c3));
...
/*2*/ std::ranges::move(views::zip(c1, c2, c3), std::back_inserter(fout));

Note, that 1 might not be supported by all file types. Can be done very wrong by doing:

fout = std::make_tuple(c1, c2, c3));

The difference is subtle and when doing wrong would result in copying the entire data into the tuple before writing it to the file. But also I haven't used either interface in a use case so there might be other valid points I am not considering.

With the code you removed you know cannot do fout = fin anymore can you? Is this tested? (It is for sequence files)

Why not? But I am very certain that there is at least one snippet that should fail if this is not working anymore?!

[h-2]

I agree the functions should be removed. You can still use assignment by range interface and do

fout = views::zip(c1, c2, c3);

No moving and back-inserters required :)

[smehringer]

I agree, we should remove the interface then. And we should add a test for the example @h-2 gave :)

[smehringer]

Nice! Just one tiny thing.

And can you merge the changelog commit into the actual commit that introduced the change? such that those appear together :)

[smehringer]

Suggested change

	#### Input/Output
	#### I/O

💅 Since this is the actual directory name ?

[joergi-w]

There was already a section Input/Output in the file above so I wanted to keep it consistent. I changed both to I/O now.

[joergi-w]

And can you merge the changelog commit into the actual commit that introduced the change? such that those appear together :)

I made two commits:

1. removes the input interface and adds the changelog entry
2. removes the output interface as well as documents and tests the zip iterator version

[smehringer]

One more thing I noticed. We do not have the tag [API]. Only

[FEATURE] Whenever you implement something new and shiny
[FIX] Whenever you fix some wrong code in the source
[DOC] Whenever you do something only(!) related to the documentation
[INFRA] Whenever you change something of the build system or CI related
[TEST] Whenever you do something related to the tests (unit or benchmark)
[MISC] Whenever it does not fit to any of the above

I think MISC is fine :) If you change the names I'll merge.

[rrahn]

No moving and back-inserters required :)

Even better.