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

[FEATURE] counting_agent for the interleaved bloom filter #2373

Merged
merged 1 commit into from
Feb 16, 2021

Conversation

eseiler
Copy link
Member

@eseiler eseiler commented Feb 10, 2021

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #2373 (4124876) into master (a1d5d4d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2373   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files         265      265           
  Lines       10806    10820   +14     
=======================================
+ Hits        10613    10627   +14     
  Misses        193      193           
Impacted Files Coverage Δ
...n3/search/dream_index/interleaved_bloom_filter.hpp 100.00% <100.00%> (ø)

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 a1d5d4d...a1b1e84. Read the comment docs.

Copy link
Member

@smehringer smehringer left a comment

Choose a reason for hiding this comment

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

We should cross reference the bulk_contains and bulk_count function in the IBF itself. Otherwise it is hard to know for the user what we provide without looking into the agents themselves.


/*!\brief Returns a seqan3::interleaved_bloom_filter::counting_agent_type to be used for counting.
* \attention Calling seqan3::interleaved_bloom_filter::increase_bin_number_to invalidates all
* seqan3::interleaved_bloom_filter::counting_agent_type constructed for this Interleaved Bloom Filter.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* seqan3::interleaved_bloom_filter::counting_agent_type constructed for this Interleaved Bloom Filter.
* `seqan3::interleaved_bloom_filter::counting_agent_type`s constructed for this Interleaved Bloom Filter.

* each thread.
*/
template <std::ranges::range value_range_t>
[[nodiscard]] counting_vector<value_t> const & count_values(value_range_t && values) & noexcept
Copy link
Member

Choose a reason for hiding this comment

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

just a thing to think about:

Since the memberhip_agent has the function bulk_contains we might want to consider naming this function
bulk_count.

@eseiler eseiler requested review from a team, SGSSGene and Irallia and removed request for a team and SGSSGene February 11, 2021 16:03
Copy link
Contributor

@Irallia Irallia left a comment

Choose a reason for hiding this comment

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

LGFM, as far as I understand what is happening here.
I have only one tiny question, but will already approve.

test/snippet/search/dream_index/counting_agent.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@SGSSGene SGSSGene left a comment

Choose a reason for hiding this comment

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

Only found 2 minor things, that are actually not wrong. So I approve! 👍

test/snippet/search/dream_index/counting_agent.cpp Outdated Show resolved Hide resolved
@eseiler eseiler force-pushed the feature/counting_agent branch 2 times, most recently from 1bcedd3 to cd510f7 Compare February 15, 2021 07:44
Copy link
Member

@marehr marehr left a comment

Choose a reason for hiding this comment

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

ty, looks good :)

return result_buffer;
}

// `bulk_count` cannot be called on a temporary, since the object the returned reference points to
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that we don't need any !\brief here

Comment on lines 14 to 16
auto const sequence_1 = "ACTGACTGACTGATC"_dna4;
auto const sequence_2 = "GTGACTGACTGACTCG"_dna4;
auto const sequence_3 = "AAAAAAACGATCGACA"_dna4;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto const sequence_1 = "ACTGACTGACTGATC"_dna4;
auto const sequence_2 = "GTGACTGACTGACTCG"_dna4;
auto const sequence_3 = "AAAAAAACGATCGACA"_dna4;
auto const sequence1 = "ACTGACTGACTGATC"_dna4;
auto const sequence2 = "GTGACTGACTGACTCG"_dna4;
auto const sequence3 = "AAAAAAACGATCGACA"_dna4;

No hard-feelings for this change, but I think we normally don't underscore before numbers.

@eseiler eseiler merged commit 09205ce into seqan:master Feb 16, 2021
@eseiler eseiler deleted the feature/counting_agent branch May 14, 2021 13:58
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.

None yet

5 participants