Skip to content
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

!! C++20 like views #238

Closed
6 of 14 tasks
rrahn opened this issue Oct 22, 2020 · 3 comments
Closed
6 of 14 tasks

!! C++20 like views #238

rrahn opened this issue Oct 22, 2020 · 3 comments
Labels
needs refinement A story that was not discussed and/or estimated by the team yet but is planned for upcoming sprints.

Comments

@rrahn
Copy link
Contributor

rrahn commented Oct 22, 2020

Description !!!

Make all views as similar as possible to the C++20 standard, since this gives us the highest chance that they will work together in composable view definitions and are stable for the future. It is also a good practice, since the standard documents how they are implemented and every developer can follow these guidelines.


Already discussed:

  • C++20 "standard" conform #49
    Core meeting 2020-12-07: Everything is done already
  • All views shall have explicit constructors https://github.com/seqan/seqan3/issues/941 still valid?
    Core meeting 2020-12-07: Yes, they should be explicit if only one argument is given, e.g. http://eel.is/c++draft/range.join which does the same.
    TODO: Make an issue for this. (Old issue is being re-used; I reopened)
  • change behaviour of take/drop according to P1739/P1664
    Core meeting 2020-12-07:
    Declare take/drop view as experimental; The reason is that the standard take/drop view seems to have the same properties.
    TODO: This should be benchmarked and if there is a regression this should be backported to the standard.
  • view::persist should downgrage ContiguousIterator/Range to RandomAccessIterator/Range because contiguous suggests you can directly access the memory whose lifetime might end, if not access via view::persist. Should we still offer view::persist or remove it entirely
    Core meeting 2020-12-07: We remove this as discussed already here https://github.com/orgs/seqan/projects/4#card-41487782
  • Give up our own definitions in once some core language concepts without std::boolean broken in 0.9.x ericniebler/range-v3#1322 is resolved. Still valid?
    Core meeting 2020-12-07: This is done, see: Simplify seqan3/std/concepts #190
  • some changes to views::pairwise_combine:
    • rename to views::all_pairs -> we decided against this
    • make view factory instead of adaptor
    • have single param interface (like now)
    • add two-param interface where all pairs between the sets are generated
    • change the tuple-type to be (seq1, seq2, id1, id2) where id1 and id2 are numbers that indicate the original IDs of the sequences

Acceptance Criteria

Tasks

Definition of Done

  • Implementation and design approved
  • Unit tests pass
  • Test coverage = 100%
  • Microbenchmarks added and/or affected microbenchmarks < 5% performance drop
  • API documentation added
  • Tutorial/teaching material added
  • Test suite compiles in less than 30 seconds (on travis)
  • Changelog entry added

@rrahn rrahn changed the title C++20 like views !! C++20 like views Oct 22, 2020
@rrahn rrahn added the needs refinement A story that was not discussed and/or estimated by the team yet but is planned for upcoming sprints. label Dec 7, 2020
@marehr
Copy link
Member

marehr commented Apr 16, 2021

2021-04-16 Core Meeting

views::pairwise_combine has a slight preferential over views::all_pairs, we'll keep the pairwise_combine name.

Wiki calls it k-combination, so in this case 2-combination which is closer to pairwise_combine.

@rrahn
Copy link
Contributor Author

rrahn commented Apr 18, 2021

This came from @h-2. I just transferred it into the new issue system.

@marehr
Copy link
Member

marehr commented May 4, 2021

From my POV done; open a new issue if needed

@marehr marehr closed this as completed May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs refinement A story that was not discussed and/or estimated by the team yet but is planned for upcoming sprints.
Projects
None yet
Development

No branches or pull requests

2 participants