Actually use PackedSeq in FlatGFA#241
Conversation
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Here are a few initial comments on the FlatGFA integration itself! I'll surely have more to say after we do a bit more disentangling of the other stuff from this PR, but this will get us started…
| use std::ops::Range; | ||
| use std::str::FromStr; | ||
| //use std::{arch::aarch64::uint8x16x3_t} | ||
|
|
||
| use std::ops::Range; |
There was a problem hiding this comment.
Looks like we can probably revert these changes.
| #[allow(clippy::len_without_is_empty)] | ||
| pub fn len(&self) -> usize { | ||
| self.seq.len() | ||
| self.seq.len as usize |
There was a problem hiding this comment.
A nice thing to do here might be to provide a len() method, distinct from the len field, that just returns a usize. That would conform to the "usual" pattern for a Rust collection type.
| // The range starts at the end of the buffer: | ||
| // [-----<end<******<start<------] | ||
| &self.data[(self.data.len() - range.end)..(self.data.len() - range.start)] | ||
| self.data.slice(SeqSpan::from_range( |
There was a problem hiding this comment.
Maybe it would be nice to create a sibling method to slice in the API that takes a Range instead of a SeqSpan? Just for tidiness in cases like this.
| pub fn get_seq(&self, seg: &Segment) -> &BStr { | ||
| self.seq_data[seg.seq].as_ref() | ||
| pub fn get_seq(&self, seg: &Segment) -> PackedSeqView<'_> { | ||
| // println!("end {}", seg.seq.end()); |
There was a problem hiding this comment.
Looks like a stray debug thing we can remove.
| } | ||
| } | ||
|
|
||
| /// A Span of a packed sequence. |
There was a problem hiding this comment.
| /// A Span of a packed sequence. | |
| /// A Span of a packed nucleotide sequence. |
|
|
||
| /// Creates a subslice of this PackedSeqView in the range of `span` | ||
| pub fn slice(&self, span: SeqSpan) -> Self { | ||
| let new_data = &self.data[span.start_byte_index()..span.end_byte_index()]; |
There was a problem hiding this comment.
You could make this kind of code a little simpler to write if, instead of having start_byte_index() and end_byte_index(), you instead had a "bigger" function called byte_range() that returns a Range<usize>.
| /// A Span of a packed sequence. | ||
| #[derive(Debug, FromBytes, IntoBytes, Clone, Copy, PartialEq, Eq, Hash, Immutable)] | ||
| #[repr(packed)] | ||
| pub struct SeqSpan { |
There was a problem hiding this comment.
Maybe it would make sense to move SeqSpan to packedseq.rs, just because it is so tightly coupled with the implementation of PackedSeqView?
| @@ -448,9 +506,37 @@ impl<'a, P: StoreFamily<'a>> GFAStore<'a, P> { | |||
|
|
|||
| /// Add a new segment to the GFA file. | |||
| pub fn add_seg(&mut self, name: usize, seq: &[u8], optional: &[u8]) -> Id<Segment> { | |||
There was a problem hiding this comment.
I think I'd suggest flipping the names of add_seg and add_seg_already_compressed around, to help more obviously convey which is the "cheap one" of the two. We could call it add_seg (for the PackedSeq version) and compress_and_add_seg, for example.
| seq: SeqSpan { | ||
| start, | ||
| len: (end - start) as u16, | ||
| }, |
There was a problem hiding this comment.
This would probably be a good place for using that From<Range> impl, if we do it.
| let mut compressed: Vec<u8> = Vec::new(); | ||
| let end_offset = compress_into_buffer(seq, &mut compressed); | ||
| let byte_span = self.seq_data.add_slice(&compressed); |
There was a problem hiding this comment.
It looks like this might entail an extra, possibly-unnecessary copy. That is, we first compress into a new Vec, compressed, then copy that into self.seq_data, and then throw away compressed.
To skip this extra step, would it work to just use compress_into_buffer on seq_data, and possibly extend that API to provide enough of a result?
|
I retitled this to better reflect is role as the successor to #214. |
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Just marking a few items in the current PR that should be split off into a separate, performance-measurement-focused PR.
There was a problem hiding this comment.
I think this belongs in a separate benchmarking PR.
There was a problem hiding this comment.
Let's revert this whitespace change.
There was a problem hiding this comment.
The stuff in tests/performance/ also belongs in a separate performance-measurement PR.
2 participants
Reduces the size of each SeqSpan from 18 bytes to 8 bytes by using u32 indices and logical indices.