[FEATURE] Adding Syncmer.hpp #15

Hosseinem · 2022-01-12T17:56:40Z

Part of #1

…yncmer.hpp

MitraDarja

Hi, I just took a quick look at the minstrobes and already added some things to improve. :)

include/minstrobe.hpp

codecov-commenter · 2022-01-18T13:57:16Z

Codecov Report

Merging #15 (ad5ddce) into master (5833945) will increase coverage by 0.14%.
The diff coverage is 97.39%.

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
+ Coverage   96.12%   96.26%   +0.14%     
==========================================
  Files           9       10       +1     
  Lines         413      697     +284     
==========================================
+ Hits          397      671     +274     
- Misses         16       26      +10

Impacted Files	Coverage Δ
include/syncmer.hpp	`97.39% <97.39%> (ø)`
src/main.cpp	`100.00% <0.00%> (ø)`
include/compare.h	`100.00% <0.00%> (ø)`
src/compare.cpp	`93.06% <0.00%> (+0.76%)`	⬆️

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 5833945...ad5ddce. Read the comment docs.

MitraDarja

Can you tell me where in the code the difference between open and closed syncmers are? I must have missed it.

include/minstrobe.hpp

include/opensyncmer.hpp

MitraDarja · 2022-01-24T17:41:40Z

include/opensyncmer.hpp

+
+		opensyncmer_position_offset = std::distance(std::begin(window_values), smallest_s_it);
+
+		if (opensyncmer_position_offset == 0) {


The ; in the end seems wrong.
And seqan style is to put the {} on a seperate line each

I'm not sure I understand which one you mean. Could you please maybe highlight where exactly?

include/opensyncmer.hpp

include/syncmer.hpp

MitraDarja · 2022-01-24T17:48:21Z

You often don`t use the tabs and indentation correctly. Please, see the example below and other seqan code.

A function should look like this:

void test()
{ // gets its own line
    auto variable = 0; // is tabed in
    if (true)
   {
        variable = 1; // tabed in even further
   }
}

Hosseinem · 2022-01-24T17:58:00Z

include/minstrobe.hpp

include/syncmer.hpp

MitraDarja

Looks already really good, but some small things need to be addressed.

I also wonder, if we should rename some things for clearification.

Kmers: Your view is universal, so you can use it for any kind of values, so maybe window size makes more sense?
Smer: Same problem as with kmer, maybe smaller_window_size is better?
Window Values: This makes sense, if we rename kmer and smer, but if we leave kmer and smer, it does not make sense to name this window values.

Maybe you have some ideas?

test/api/syncmer_test.cpp

MitraDarja · 2022-02-10T11:25:59Z

include/syncmer.hpp

+/*!\brief Computes syncmers for a range of comparable values. A syncmer is a kmer that has its smallest smer
+ * (s < k) at its start or end. An open-syncmer has its smer at its start.
+ * \tparam urng_t The type of the first range being processed. See below for requirements. [template
+ *                 parameter is omitted in pipe notation]
+ * \param[in] urange1     The first input range to process. Must model std::ranges::viewable_range and
+ *                        std::ranges::forward_range.
+ * \param[in] urange2     The second input range to process. Must model std::ranges::viewable_range and
+ *                        std::ranges::forward_range.
+ * \param[in] smer_size The s-mer size used.
+ * \param[in] kmer_size The k-mer size used.
+ * \returns A range of std::totally_ordered where each value is ... See below for the
+ *          properties of the returned range.
+ * \ingroup search_views


This is not correct, this describes what you do when you use kmer_hash and syncmers together but the syncmer function itself, just gets two views (which can be not based on sequences) and then reports the values, when the smallest value in range is at a certain position.

This text can be used for a syncmer_hash view so maybe don't delete it and store it in a different file. :)

What about this:
Computes syncmers for a range of comparable values. A syncmer is a window with size k that has
its smallest subwindow with size s (s < k) at its start or end. An open-syncmer has its smallest subwindow at its start.

Now, that I think about it, you don`t need kmer and smer size, you can do the calculation for w-size before calling the syncmer function itself.
Then a syncmer would be a hash value, if the minimum of the sub_window_values is at the front or at the end...

include/syncmer.hpp

MitraDarja · 2022-02-10T11:30:07Z

include/syncmer.hpp

+    * Looks at the number of values per window in two ranges, returns the smallest in smer and returns the corresponding
+    * kmer from the other range as syncmer and shifts then by one to repeat this action.


It does not return the smer, it returns a kmer, if its smallest smer is at a certain position.

MitraDarja · 2022-02-10T11:31:34Z

include/syncmer.hpp

+    //!\brief Compare to another basic_iterator.
+    friend bool operator==(basic_iterator const & lhs, basic_iterator const & rhs)
+    {
+        return (lhs.urng1_iterator == rhs.urng1_iterator);


Shouldn`t this also include the second iterator and the size of window values?

MitraDarja · 2022-02-10T11:33:06Z

include/syncmer.hpp

+    }
+
+    //!\brief Returns new window value.
+    auto window_value() const


I don`t think you need this function, just use *urng1_iterator directly.

MitraDarja · 2022-02-10T11:33:32Z

include/syncmer.hpp

+        auto smallest_s_it = std::ranges::min_element(window_values, std::less<value_type>{});
+        syncmer_position_offset = std::distance(std::begin(window_values), smallest_s_it);
+
+


Delete this empty line

MitraDarja · 2022-02-10T11:34:00Z

include/syncmer.hpp

+            {
+                auto syncmer_it = urng2_iterator;
+                syncmer_value = *syncmer_it;
+            };


Why is there a ; here?

This is still not resolved.

You also have these in other positions of the code, I think that you should delete them.

MitraDarja · 2022-02-10T11:35:44Z

include/syncmer.hpp

+            if constexpr (!opensyncmer)
+            {
+            auto syncmer_it = urng2_iterator;
+            syncmer_value = *syncmer_it;
+            return true;
+            };


Intendation is incorrect.

MitraDarja

Looks good, just some really minor things. Sorry. 💅

MitraDarja · 2022-02-11T16:33:11Z

include/syncmer.hpp

+            {
+                auto syncmer_it = urng2_iterator;
+                syncmer_value = *syncmer_it;
+            };


This is still not resolved.

MitraDarja · 2022-02-11T16:33:38Z

include/syncmer.hpp

+            {
+                auto syncmer_it = urng2_iterator;
+                syncmer_value = *syncmer_it;
+            };


You also have these in other positions of the code, I think that you should delete them.

include/syncmer.hpp

Hosseinem and others added 5 commits January 10, 2022 14:27

[FEATURE] Adding minstrobe and syncmer preliminary implementations

edaac4e

[FIX] fixed some typos in minstrobe.hpp file

6ef4bf6

[FIX] Syncmer implementation with two ranges

e4039bd

Merge branch 'master' of https://github.com/Hosseinem/minions

431bc1b

[FEATURE,FIX] Adding opensyncmer.hpp file and fixing case of tie in s…

2623d87

…yncmer.hpp

MitraDarja requested changes Jan 18, 2022

View reviewed changes

Hossein Eizadi Moghadam added 2 commits January 20, 2022 18:40

[FIX] Some typos and comments fixed.

8502f49

[FIX] Some typos fixed

27b73fa

MitraDarja requested changes Jan 24, 2022

View reviewed changes

Hosseinem closed this Jan 24, 2022

Hosseinem reopened this Jan 24, 2022

Hossein Eizadi Moghadam added 3 commits January 24, 2022 22:20

[FIX] problems resolved.

264ae8b

[FIX] opensyncmer.hpp removed.

03af103

[FIX] Problems resolved.

dd43b6d

MitraDarja requested changes Jan 25, 2022

View reviewed changes

Hossein Eizadi Moghadam and others added 4 commits January 25, 2022 19:09

[FIX] constexpr added to syncmer and minstrobe iterator renamed

5054df3

[FIX] Syncmer documentation fixed.

38fedc2

[FEATURE,FIX] syncmer_test added and syncmer.hpp edited.

44752d1

[FIX] syncmer_test files fixed and minstrobe removed.

730d04b

Hosseinem changed the title ~~[FEATURE] Adding Minstrobe.hpp and Syncmer.hpp~~ [FEATURE] Adding Syncmer.hpp Feb 9, 2022

MitraDarja requested changes Feb 10, 2022

View reviewed changes

Hosseinem added 2 commits February 10, 2022 16:19

[FIX] Smer and Kmer removed.

b7c791d

[FIX] Syncmer_test fixed according to syncmer.hpp

47a419d

MitraDarja requested changes Feb 11, 2022

View reviewed changes

[FIX] Some errors fixed.

ad5ddce

Hosseinem requested a review from MitraDarja February 11, 2022 17:31

MitraDarja approved these changes Feb 11, 2022

View reviewed changes

MitraDarja merged commit e198317 into seqan:master Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Adding Syncmer.hpp #15

[FEATURE] Adding Syncmer.hpp #15

Hosseinem commented Jan 12, 2022 •

edited by MitraDarja

Loading

MitraDarja left a comment

codecov-commenter commented Jan 18, 2022 •

edited

Loading

MitraDarja left a comment

MitraDarja Jan 24, 2022

Hosseinem Jan 24, 2022

MitraDarja commented Jan 24, 2022

Hosseinem commented Jan 24, 2022

MitraDarja left a comment •

edited

Loading

MitraDarja Feb 10, 2022

Hosseinem Feb 10, 2022

MitraDarja Feb 10, 2022

MitraDarja Feb 10, 2022

MitraDarja Feb 10, 2022

MitraDarja Feb 10, 2022 •

edited

Loading

MitraDarja Feb 10, 2022

MitraDarja Feb 10, 2022

MitraDarja Feb 11, 2022

MitraDarja Feb 11, 2022

MitraDarja Feb 10, 2022

MitraDarja left a comment

MitraDarja Feb 11, 2022

MitraDarja Feb 11, 2022


		opensyncmer_position_offset = std::distance(std::begin(window_values), smallest_s_it);

		if (opensyncmer_position_offset == 0) {

		* Looks at the number of values per window in two ranges, returns the smallest in smer and returns the corresponding
		* kmer from the other range as syncmer and shifts then by one to repeat this action.

		auto smallest_s_it = std::ranges::min_element(window_values, std::less<value_type>{});
		syncmer_position_offset = std::distance(std::begin(window_values), smallest_s_it);

[FEATURE] Adding Syncmer.hpp #15

[FEATURE] Adding Syncmer.hpp #15

Conversation

Hosseinem commented Jan 12, 2022 • edited by MitraDarja Loading

MitraDarja left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 18, 2022 • edited Loading

Codecov Report

MitraDarja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitraDarja commented Jan 24, 2022

Hosseinem commented Jan 24, 2022

MitraDarja left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitraDarja Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitraDarja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hosseinem commented Jan 12, 2022 •

edited by MitraDarja

Loading

codecov-commenter commented Jan 18, 2022 •

edited

Loading

MitraDarja left a comment •

edited

Loading

MitraDarja Feb 10, 2022 •

edited

Loading