-
Notifications
You must be signed in to change notification settings - Fork 83
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
[TEST] implement sequence generator #1985
Conversation
d61513a
to
5cde5e6
Compare
1aec6de
to
997f710
Compare
30959f1
to
fe7b96e
Compare
d6a32ec
to
6e1c646
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2nd Round :)
32f6f0c
to
649e389
Compare
I get an error message in the seqan2 part of the sequence_generator_test. Should I just remove this part as it will be covered later? In file included from .../seqan3/test/unit/test/sequence_generator_test.cpp:12:
.../seqan3/test/include/seqan3/test/performance/sequence_generator.hpp: In instantiation of 'sequence_t seqan3::test::random_sequence_generator<sequence_t>::operator()(generator_t&&) const [with generator_t = std::mersenne_twister_engine<long long unsigned int, 64, 312, 156, 31, 13043109905998158313, 29, 6148914691236517205, 17, 8202884508482404352, 37, 18444473444759240704, 43, 6364136223846793005>&; sequence_t = seqan::String<seqan::SimpleType<unsigned char, seqan::Dna_>, seqan::Alloc<> >]':
..../seqan3/test/unit/test/sequence_generator_test.cpp:101:68: required from here
.../seqan3/test/include/seqan3/test/performance/sequence_generator.hpp:81:33: error: use of invalid variable template 'alphabet_size<alphabet_t>'
81 | max_alphabet_rank = alphabet_size<alphabet_t> - 1ull;
| ^~~~~~~~~~~~~~~~~~~~~~~~~ |
Seems like you are using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
next round :)
If you approve I will rebase and clear up the history. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, we are nearly done :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, we are very close to finish :)
8768c7a
to
18e21ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you :) Second Reviewer do your work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already a big start towards a better design :).
I found some issues that should be addressed. Otherwise it looks fine to me.
Thank you!
std::uniform_int_distribution<size_t> dis_alpha(0ull, max_val); | ||
std::uniform_int_distribution<size_t> dis_length(len - variance, len + variance); | ||
public: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~random_sequence_generator() = default; //!< Defaulted | ||
|
||
/*!\brief Initialises a random sequence generator which generates sequences with a given mean size. | ||
* \param[in] size The size of the random sequence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* \param[in] size The size of the random sequence. | |
* \param[in] size The size of the random sequence. |
|
||
/*!\brief Initialises a random sequence generator which generates sequences with a given mean size. | ||
* \param[in] size The size of the random sequence. | ||
* \param[in] size_variance The variance of the random sequence size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* \param[in] size_variance The variance of the random sequence size. | |
* \param[in] size_variance The variance of the random sequence size (defaults to `0`). |
*/ | ||
|
||
/*!\brief Returns a random sequence for a set size (and variance). | ||
* \param[in,out] random_generator e.g. std::mt19937 with a seed of 42 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* \param[in,out] random_generator e.g. std::mt19937 with a seed of 42 | |
* \param[in,out] random_generator, e.g. std::mt19937 with a seed of 42 |
|
||
/*!\brief Returns a random sequence for a set size (and variance). | ||
* \param[in,out] random_generator e.g. std::mt19937 with a seed of 42 | ||
* \returns a random generated sequence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* \returns a random generated sequence. | |
* \returns a generated random sequence. |
std::uniform_int_distribution<size_t> alphabet_rank_distribution(0ull, max_alphabet_rank); | ||
std::uniform_int_distribution<size_t> sequence_size_distribution(size - size_variance, size + size_variance); | ||
|
||
size_t sequence_size = sequence_size_distribution(random_generator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size_t sequence_size = sequence_size_distribution(random_generator); | |
size_t const sequence_size = sequence_size_distribution(random_generator); |
|
||
size_t sequence_size = sequence_size_distribution(random_generator); | ||
sequence_t random_sequence(sequence_size); | ||
std::ranges::generate(random_sequence, [&]() -> alphabet_t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::ranges::generate(random_sequence, [&]() -> alphabet_t | |
std::ranges::generate(random_sequence, [&] () -> alphabet_t |
std::uniform_int_distribution<size_t> sequence_size_distribution(size - size_variance, size + size_variance); | ||
|
||
size_t sequence_size = sequence_size_distribution(random_generator); | ||
sequence_t random_sequence(sequence_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pheww, can we actually assume that this does in fact always does what we expect? I think this is not handled properly by the concepts yet. The biggest problem that I see is handling the SeqAn2 sequence types which do not have push_back or resize member functions. So I guess later on we need to use a helper function to differentiate between sequences that have a resize member function. For now I propose to add this inline requires clause in the head of the class template:
requires requires (sequence_t & s) { {s.resize()}; }
And then use here the resize member function instead of the constructor.
18e21ed
to
7c7cc61
Compare
template <typename sequence_t> | ||
//!\cond | ||
requires (std::ranges::output_range<sequence_t, std::ranges::range_value_t<sequence_t>> && | ||
requires (sequence_t & s) { {s.resize(1)}; } ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requires (sequence_t & s) { {s.resize(1)}; } ) | |
requires (sequence_t & s) { {s.resize(1)}; }) |
else | ||
sequence.push_back(assign_rank_to(dis_alpha(gen), alphabet_t{})); | ||
max_alphabet_rank = alphabet_size<alphabet_t> - 1ull; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max_alphabet_rank = alphabet_size<alphabet_t> - 1ull; | |
max_alphabet_rank = seqan3::alphabet_size<alphabet_t> - 1ull; |
if constexpr (std::is_same_v<alphabet_t, uint64_t>) | ||
return alphabet_rank_distribution(random_generator); | ||
else | ||
return assign_rank_to(alphabet_rank_distribution(random_generator), alphabet_t{}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return assign_rank_to(alphabet_rank_distribution(random_generator), alphabet_t{}); | |
return seqan3::assign_rank_to(alphabet_rank_distribution(random_generator), alphabet_t{}); |
7c7cc61
to
d6012fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff only. Please rebase right away. I approve already.
*/ | ||
|
||
/*!\brief Returns a random sequence for a set size (and variance). | ||
* \param[in,out] random_generator, e.g. std::mt19937 with a seed of 42 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* \param[in,out] random_generator, e.g. std::mt19937 with a seed of 42 | |
* \param[in,out] random_generator, e.g. std::mt19937 with a seed of 42. |
|
||
/*!\brief Returns a random sequence for a set size (and variance). | ||
* \param[in,out] random_generator, e.g. std::mt19937 with a seed of 42 | ||
* \returns a generated random sequence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* \returns a generated random sequence. | |
* \returns A generated random sequence. |
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
d6012fc
to
c7bb6d2
Compare
resolves part of seqan/product_backlog#12
The first commit will be done by another PR: #1995 ([TEST] Repair seqan2 tests)