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

Documentation/views #103

Merged
merged 3 commits into from
Aug 8, 2017
Merged

Documentation/views #103

merged 3 commits into from
Aug 8, 2017

Conversation

h-2
Copy link
Member

@h-2 h-2 commented Aug 4, 2017

@seqan/all note that I made some changes to https://github.com/seqan/seqan3/wiki/Documentation#configuration to generate nicer inheritance graphs. Please include them in your next doxygen builds

@h-2 h-2 assigned rrahn Aug 4, 2017
@h-2 h-2 requested review from rrahn and sarahet August 4, 2017 16:25
@h-2 h-2 force-pushed the documentation/views branch from 7f91594 to 18c9b0f Compare August 4, 2017 17:22
@h-2 h-2 mentioned this pull request Aug 4, 2017
* | | `irng_t` (range input type) | `rrng_t` (range return type) |
* |---------------------|-------------------------------|--------------------------------------------------------------------------------------------------|
* | range | seqan3::input_range_concept | seqan3::view_concept + all range concepts met by `irng_t` |
* | `range_reference_t` | seqan3::nucleotide_concept | `std::remove_reference_t<range_reference_t<irng_t>>`, *usually* == `range_value_type_t<irng_t>` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what cases is range_value_type_t<irng_t> != range_value_type_t<rrng_t>?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that depends entirely on the view. If you convert a range of dna5 to dna4, than of course the range_value_type_t of the input is dna5 and the range_value_type_t of the output is dna4.
But more important than the range_value_type_t is the range_reference_t, because (*it) and operator[] (all operations used really) return the range_reference_t, not the range_value_type_t. Now this is more interesting, because normally (say on a vector of dna5), the range_reference_t is dna5 &. If you apply ranges::view::reverse the reference type is preserved fully, i.e. the range_reference_t<rrng_t> is also dna5 &. This means among other things that you can use the reference value to modify the positions in the original range (unless of course the original range was const, in which case the reference type would have been dna5 const &).

On the other hand, view::complement of course cannot preserve regular references to the original range, because it produces new values of the nucleotide type (the complement value). That's why the range_reference_t<rrng_t> in this case is equal to range_reference_t<irngt_t> but with the reference removed (which may or may not be identical to the range_value_type_t<irng_t>). This means you cannot use the elements in the returned range to modify the original range.
Note that you can achieve a modifying-the-original-behaviour even here, if you create a proxy type that looks like the value_type, but contains references to the original which it "re-complements" and writes back when modified. But that is unrelated to the rest and I didn't see it as useful for this implementation.

I will add a section to the general view documentation that tries to explain this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the nice explanation :)

* | | `irng_t` (range input type) | `rrng_t` (range return type) |
* |---------------------|-----------------------------------------------|-----------------------------------------------------------|
* | range | seqan3::input_range_concept | seqan3::view_concept + all range concepts met by `irng_t` |
* | `range_reference_t` | seqan3::underlying_char_t<alphabet_t> | `alphabet_t` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a little confusing that concepts of ranges are defined in that table and concept of alphabet_t is stated before that. Can we combine this somehow?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't fully understand what you mean by "concepts of ranges are defined in that table".
But I agree that it is not ideal, that the cell content sometimes is a concrete type (in which case it means is of this type) and sometimes a concept (in which case it means requires/satisfies this concept).
But I couldn't think of a way of making this clearer without cluttering the table. I will add more documentation to the general page, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we do both? An extra cell for types and a cell for concepts, always?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make an example of what you mean? I think it would be confusing, because it would double the table size and always half of the cells would be empty 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yes I meant exactly that and you are right the size would double and cells might stay empty a lot. So another suggestion: Could we just explicitly write into the cell satisfies concept or is of type type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just explicitly write into the cell satisfies concept or is of type type?

Yes, we could. OTOH it is very verbose. All our types and type metafunctions end in _type or _t and all our concepts end in _concept so I thought this would be clear. Also concepts will be used more and more like types so people will get used to the distinction (or not even care).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have merged this now, we can maybe re-evaluate it at a later point in time. Or you can make a PR and convince me 😄

@h-2 h-2 force-pushed the documentation/views branch from 18c9b0f to 868992a Compare August 7, 2017 13:45
@h-2
Copy link
Member Author

h-2 commented Aug 7, 2017

I have added lots more documentation to the views sub-module page.

@h-2 h-2 force-pushed the documentation/views branch from 868992a to c6d3be9 Compare August 8, 2017 13:18
@h-2 h-2 merged commit 1e7b695 into seqan:master Aug 8, 2017
@h-2 h-2 mentioned this pull request Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants