[FEATURE] Modularisation clustering #49

Irallia · 2021-02-04T13:47:17Z

Resolves #41.
I recommend reviewing commit wise.

What have I done:

[FEATURE] Add parser option for clustering
First I added the new parser option -c with 4 different clustering methods. (none is implemented)
[FEATURE] Implement simple clustering method.
Then I implemented a simple clustering method as an example module. This method compares junctions, and if they are equal, one will be removed from the junction list.
[MISC] Add 'not implemented' messages.
This commit just added some more debug messages.
[FEATURE] Add a 'supporting reads' count.
As some junctions are removed, I have included a counter for how many reads support a junction.

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

MitraDarja

Some small stuff.
Plus, you could use an enum for the clustering methods, if you want. :)

src/detect_breakends.cpp

include/junction.hpp

src/detect_breakends/junction_detection.cpp

MitraDarja · 2021-02-04T17:56:37Z

test/api/junction_detection_test.cpp

+// Reference\tm2257/8161/CCS\t41972616\tForward\tRead \t0\t2294\tForward\tchr21
+// INS from Primary Read - Sequence Type: Reference; Sequence Name: m2257/8161/CCS; Position: 41972616; Orientation: Reverse
+//                         Sequence Type: Read; Sequence Name: 0; Position: 3975; Orientation: Reverse
+//                         Chromosome: chr21
+// Reference\tchr22\t17458417\tForward\tReference\tchr21\t41972615\tForward\tm41327/11677/CCS
+// BND from SA Tag - Sequence Type: Reference; Chromosome: chr22; Position: 17458417; Orientation: Forward
+//                   Sequence Type: Reference; Chromosome: chr21; Position: 41972615; Orientation: Forward
+//                   Sequence Name: m41327/11677/CCS
+


Why is this here?

I thought I'd add an explanation of the string as I stumbled over it and was confused as to what the parts of the string were.

Ok, makes sense :)

I'm confused by this string:

// Reference\tm2257/8161/CCS\t41972616\tForward\tRead \t0\t2294\tForward\tchr21

It should consist of breakend1, breakend2 and the read name it was detected from. However, the read name here is chr21 and the reference chromosome of breakend1 is m2257/8161/CCS\t41972616. Those are swapped but I don't immediately see why 😕

test/cli/detect_breakends_cli_test.cpp

src/detect_breakends/junction_detection.cpp

test/cli/detect_breakends_cli_test.cpp

include/junction.hpp

MitraDarja

LGTM :)

MitraDarja · 2021-02-05T15:15:39Z

test/api/junction_detection_test.cpp

+// Reference\tm2257/8161/CCS\t41972616\tForward\tRead \t0\t2294\tForward\tchr21
+// INS from Primary Read - Sequence Type: Reference; Sequence Name: m2257/8161/CCS; Position: 41972616; Orientation: Reverse
+//                         Sequence Type: Read; Sequence Name: 0; Position: 3975; Orientation: Reverse
+//                         Chromosome: chr21
+// Reference\tchr22\t17458417\tForward\tReference\tchr21\t41972615\tForward\tm41327/11677/CCS
+// BND from SA Tag - Sequence Type: Reference; Chromosome: chr22; Position: 17458417; Orientation: Forward
+//                   Sequence Type: Reference; Chromosome: chr21; Position: 41972615; Orientation: Forward
+//                   Sequence Name: m41327/11677/CCS
+


Ok, makes sense :)

eldariont

The addition of cluster method to the parser looks good.

However, I think that the implementation of the simple clustering method (including the corresponding changes to the junction class) could be improved. Proviously, the junction class represented a single junction detected from a single read. Now, it is also used to represent clusters of junctions. IMO, we should represent clusters of junctions with a separate class or a std::vector. Then, we could keep all the cluster-related information, such as the number of supporting reads, the read names, etc. separated from the information on a single junction.

eldariont · 2021-02-05T16:40:19Z

include/detect_breakends/junction_detection.hpp

@@ -16,6 +17,10 @@
 *                                                           2: split_read,
 *                                                           3: read_pairs,
 *                                                           4: read_depth)
+ * \param clustering_method list of Methods for clustering junctions (0: simple_clustering


Suggested change

* \param clustering_method list of Methods for clustering junctions (0: simple_clustering

* \param clustering_method method for clustering junctions (0: simple_clustering

include/junction.hpp

eldariont · 2021-02-05T17:05:03Z

test/api/junction_detection_test.cpp

+// Reference\tm2257/8161/CCS\t41972616\tForward\tRead \t0\t2294\tForward\tchr21
+// INS from Primary Read - Sequence Type: Reference; Sequence Name: m2257/8161/CCS; Position: 41972616; Orientation: Reverse
+//                         Sequence Type: Read; Sequence Name: 0; Position: 3975; Orientation: Reverse
+//                         Chromosome: chr21
+// Reference\tchr22\t17458417\tForward\tReference\tchr21\t41972615\tForward\tm41327/11677/CCS
+// BND from SA Tag - Sequence Type: Reference; Chromosome: chr22; Position: 17458417; Orientation: Forward
+//                   Sequence Type: Reference; Chromosome: chr21; Position: 41972615; Orientation: Forward
+//                   Sequence Name: m41327/11677/CCS
+


I'm confused by this string:

// Reference\tm2257/8161/CCS\t41972616\tForward\tRead \t0\t2294\tForward\tchr21

It should consist of breakend1, breakend2 and the read name it was detected from. However, the read name here is chr21 and the reference chromosome of breakend1 is m2257/8161/CCS\t41972616. Those are swapped but I don't immediately see why 😕

test/api/junction_detection_test.cpp

Irallia · 2021-02-08T13:42:11Z

The addition of cluster method to the parser looks good.

However, I think that the implementation of the simple clustering method (including the corresponding changes to the junction class) could be improved. Proviously, the junction class represented a single junction detected from a single read. Now, it is also used to represent clusters of junctions. IMO, we should represent clusters of junctions with a separate class or a std::vector. Then, we could keep all the cluster-related information, such as the number of supporting reads, the read names, etc. separated from the information on a single junction.

Yes, thats definetly a good idea. You said, that you worked on a cluster method. My idea with this simple method was to create a placeholder for a real one. I will write a new issue for decouple junctions and cluster of junctions. Otherwise it would blow up this PR I think.
-> #51

eldariont

Cool, thanks for creating the two new issues. I will have a look. As far as I'm concerned, this PR can be merged now :)

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

[DOC] Correct path to binary

Irallia added 2 commits February 4, 2021 11:04

[FEATURE] Add parser option for clustering

d86f263

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

[FEATURE] Implement simple clustering method.

d4f7356

Irallia requested a review from a team February 4, 2021 13:47

Irallia self-assigned this Feb 4, 2021

Irallia requested review from MitraDarja and removed request for a team February 4, 2021 13:47

MitraDarja suggested changes Feb 4, 2021

View reviewed changes

Irallia requested a review from MitraDarja February 5, 2021 12:52

MitraDarja approved these changes Feb 5, 2021

View reviewed changes

Irallia requested a review from eldariont February 5, 2021 15:26

eldariont reviewed Feb 5, 2021

View reviewed changes

Irallia changed the title ~~Feature/modularisation clustering~~ [FEATUrE] Modularisation clustering Feb 8, 2021

Irallia changed the title ~~[FEATUrE] Modularisation clustering~~ [FEATURE] Modularisation clustering Feb 8, 2021

Irallia mentioned this pull request Feb 8, 2021

[BUG] Mixed up output string. #52

Closed

Irallia requested a review from eldariont February 8, 2021 14:47

eldariont approved these changes Feb 8, 2021

View reviewed changes

Irallia added 3 commits February 8, 2021 18:18

[MISC] Add 'not implemented' messages.

3dfd5a0

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

[FEATURE] Add a 'supporting reads' count.

9d19841

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

[FEATURE] Change cluster parsing argument into an enum.

318628b

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>

Irallia force-pushed the FEATURE/modularisation_clustering branch from 0328007 to 318628b Compare February 8, 2021 17:23

Irallia merged commit 9dbeb6a into seqan:master Feb 8, 2021

Irallia pushed a commit to Irallia/iGenVar that referenced this pull request Sep 17, 2021

Merge pull request seqan#49 from seqan/doc-execute

bd8292c

[DOC] Correct path to binary

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Modularisation clustering #49

[FEATURE] Modularisation clustering #49

Irallia commented Feb 4, 2021

MitraDarja left a comment

MitraDarja Feb 4, 2021

Irallia Feb 5, 2021

MitraDarja Feb 5, 2021

eldariont Feb 5, 2021

MitraDarja left a comment

MitraDarja Feb 5, 2021

eldariont left a comment

eldariont Feb 5, 2021

eldariont Feb 5, 2021

Irallia commented Feb 8, 2021 •

edited

eldariont left a comment

	* \param clustering_method list of Methods for clustering junctions (0: simple_clustering
	* \param clustering_method method for clustering junctions (0: simple_clustering

[FEATURE] Modularisation clustering #49

[FEATURE] Modularisation clustering #49

Conversation

Irallia commented Feb 4, 2021

MitraDarja left a comment

Choose a reason for hiding this comment

MitraDarja Feb 4, 2021

Choose a reason for hiding this comment

Irallia Feb 5, 2021

Choose a reason for hiding this comment

MitraDarja Feb 5, 2021

Choose a reason for hiding this comment

eldariont Feb 5, 2021

Choose a reason for hiding this comment

MitraDarja left a comment

Choose a reason for hiding this comment

MitraDarja Feb 5, 2021

Choose a reason for hiding this comment

eldariont left a comment

Choose a reason for hiding this comment

eldariont Feb 5, 2021

Choose a reason for hiding this comment

eldariont Feb 5, 2021

Choose a reason for hiding this comment

Irallia commented Feb 8, 2021 • edited

eldariont left a comment

Choose a reason for hiding this comment

Irallia commented Feb 8, 2021 •

edited