Splitter part 1#1509
Conversation
Itolstoganov
commented
Aug 4, 2025
Itolstoganov
commented
- Barcode index structure
- Contracted graph structure
- Scaffold graph vertices can store either edges or paths
146bde1 to
77e3265
Compare
asl
left a comment
asl
left a comment
There was a problem hiding this comment.
See comments, overall good, though would be better to have some things fixed :)
| @@ -0,0 +1,18 @@ | |||
| ############################################################################ | |||
| # Copyright (c) 2019 Saint Petersburg State University | |||
There was a problem hiding this comment.
Let us use default copyright headers here and everywhere :)
| typedef std::map<VertexId, std::unordered_set<ScaffoldVertex>>::value_type value_type; | ||
|
|
||
| AdjacencyMap() = default; | ||
| AdjacencyMap(const VertexId &vertex, const ScaffoldVertex &edge) : data_({{vertex, {edge}}}) {} |
There was a problem hiding this comment.
Let us pass small things (e.g. VertexId / EdgeId) by value – they are trivially copyable small things and we're outlining the methods, so there is no way these will be inlined.
| typedef debruijn_graph::VertexId VertexId; | ||
| typedef debruijn_graph::EdgeId EdgeId; | ||
| typedef scaffold_graph::ScaffoldVertex ScaffoldVertex; | ||
| typedef std::map<VertexId, std::unordered_set<ScaffoldVertex>>::const_iterator const_iterator; |
There was a problem hiding this comment.
I'd have a single typename for a map type and use dependent types from it. This way changing map type would require a single-line change.
| ContractedGraph(ContractedGraph &&other) = default; | ||
|
|
||
| void InsertVertex(const VertexId &vertex); | ||
| void InsertEdge(const VertexId &head, const VertexId &tail, const ScaffoldVertex &edge); |
There was a problem hiding this comment.
See all comments above about AdjacencyMap
| visited.insert(start); | ||
| ScaffoldVertex curr_vertex = start; | ||
| while(edge_to_next.find(curr_vertex) != edge_to_next.end()) { | ||
| curr_vertex = edge_to_next.at(curr_vertex); |
There was a problem hiding this comment.
You can use the value from the iterator to save on double lookups.
| }; | ||
|
|
||
| inline std::ostream& operator <<(std::ostream& os, const SimpleBarcodeInfo& info) | ||
| { |
| BarcodeId barcode; | ||
| FrameBarcodeInfo info(number_of_frames_); | ||
| BinRead(str, barcode, info); | ||
| barcode_distribution_.insert({std::move(barcode), std::move(info)}); |
| }; | ||
|
|
||
| inline std::ostream& operator <<(std::ostream& os, const FrameBarcodeInfo& info) | ||
| { |
| } | ||
| } | ||
| INFO("Number of encountered barcodes: " << barcodes.size()); | ||
| std::random_device rd; |
There was a problem hiding this comment.
Can it be downsampled deterministically? This would yield different result on different platforms and on different seeds.
b363cbd to
61c4d37
Compare
| codes_[barcode] = encoder_size + start_; | ||
| return encoder_size; | ||
| BarcodeId find_or_insert(const std::string &barcode_string) { | ||
| try { |
There was a problem hiding this comment.
Do you really need to throw / catch exceptions in the normal flow? Can't you just do something like this:
size_t num_barcodes = barcodes_map_.size();
auto [it, inserted] = barcodes_map_.insert(barcode_string, num_barcodes + start_);
return it->second;(you original code used num_barcodes + start_ as value, but returned num_barcodes was it intentional?)
There was a problem hiding this comment.
Oh, realized that it it is a concurrent hash table. You'd use upsert here.
There was a problem hiding this comment.
Fixed (not sure how to do this in a single lookup though, upsert does not seem to help)
0011903 to
cac7943
Compare
… in BarcodeEncoder
2 participants
