taxonomic classification skeleton with the RMQ preprocessing #346

heracle · 2021-07-30T19:56:04Z

This PR draws the entire structure of the taxonomic classification approach (without CLI cmds):

The header script tax_classifier.hpp contains the description of all the taxonomic classification classes:

TaxonomyBase -> implements the methods and fields that will be used by both classification options (via anno matrix or via taxDB)
TaxonomyClsAnno -> implements the methods for via anno matrix option
TaxonomyClsImportDB -> implements the methods for via taxDB option

In this PR there are fully implemented the following methods that are used for the preprocessing steps:

label to taxid parsing (which is different for GenBank or Kraken2 sequence labels)
parsing the taxonomic tree file
parsing the accession version to taxid lookup table file
DFS and RMQ precomputation

heracle · 2021-07-30T20:21:43Z

Please let me know if the structure of the classes and the files looks ok:

At this moment there is one class TaxonomyBase with the methods that will be used by both classification options. Then TaxonomyClsImportDB and TaxonomyClsAnno do a public inheritance.
There is a file label_to_taxid.cpp where there are implemented all the methods regarding the label parsing. I created a separate file just for the reason to be able to extend the enum LabelType by changing one file only

metagraph/src/annotation/taxonomy/tax_classifier.hpp

metagraph/src/annotation/taxonomy/tax_classifier.cpp

metagraph/src/annotation/taxonomy/tax_classifier.hpp

metagraph/src/annotation/taxonomy/tax_classifier.cpp

Signed-off-by: Radu Muntean <[email protected]>

metagraph/src/annotation/taxonomy/label_to_taxid.cpp

metagraph/src/annotation/taxonomy/tax_classifier.hpp

metagraph/src/annotation/taxonomy/tax_classifier.cpp

heracle · 2021-08-05T10:05:33Z

Thank you very much for the reviews!

karasikov

Please close resolved comments and also do a pass to fix all similar cases.

Signed-off-by: Radu Muntean <[email protected]>

heracle · 2021-08-05T18:48:55Z

Cool. Please let me know about anything not proper. Usually, I am trying my best to pass through the entire PR and fix all the similar cases, but for some of them, maybe I am not generalizing too well yet

metagraph/src/annotation/taxonomy/tax_classifier.cpp

metagraph/tests/annotation/taxonomy/test_taxonomy.cpp

metagraph/src/annotation/taxonomy/tax_classifier.hpp

metagraph/src/annotation/taxonomy/tax_classifier.cpp

Signed-off-by: Radu Muntean <[email protected]>

metagraph/src/annotation/taxonomy/tax_classifier.cpp

metagraph/src/annotation/taxonomy/tax_classifier.hpp

karasikov · 2021-08-08T13:46:39Z

metagraph/src/annotation/taxonomy/tax_classifier.cpp

+bool TaxonomyBase::get_taxid_from_label(const std::string &label, TaxId *taxid) const {
+    if (label_type == TAXID) {
+        *taxid = std::stoul(utils::split_string(label, "|")[1]);
+        return true;
+    } else if (TaxonomyBase::label_type == GEN_BANK) {
+        std::string acc_version = get_accession_version_from_label(label);
+        if (not accversion_to_taxid_map.count(acc_version)) {
+            return false;
+        }
+        *taxid = accversion_to_taxid_map.at(acc_version);
+        return true;
+    }
+
+    logger->error("Error: Could not get the taxid for label {}", label);
+    exit(1);
+}


I'm confused by these helpers.

You are setting label_type somewhere else, then it's used here. So it requires a certain sequence of these calls right?
What is the point to separate these functions then?

I will need to use this label_type from different enterpoints from the CLI cmd. Currently, it is computed in the TaxonomyClsAnno constructor and will be further used inside tax_class(sequence) method where we will need to iterate the column labels and take the associated taxids. If you think that it would be more clear, I can move it as part of the tax_class and run it multiple times (should be fast, I guess).

metagraph/src/annotation/taxonomy/tax_classifier.cpp

Signed-off-by: Radu Muntean <[email protected]>

metagraph/src/annotation/taxonomy/tax_classifier.hpp

karasikov · 2021-08-10T21:28:18Z

metagraph/src/annotation/taxonomy/tax_classifier.cpp

+        }
+    }
+    if (num_taxid_failed) {
+        logger->warn("During the tax_tree_filepath {} parsing, {} taxids were not found out of {} total evaluations.",


Suggested change

logger->warn("During the tax_tree_filepath {} parsing, {} taxids were not found out of {} total evaluations.",

logger->warn("During the tax_tree_filepath {} parsing, {} taxids were not found out of {} total evaluations",

Regarding logger messages, I am not very sure in what context to add the period and when to remove it. Can you please list some small examples to understand how to write these in the future?

metagraph/src/annotation/taxonomy/tax_classifier.cpp

karasikov · 2021-08-10T21:36:08Z

metagraph/src/annotation/taxonomy/tax_classifier.cpp

+bool TaxonomyBase::get_taxid_from_label(const std::string &label, TaxId *taxid) const {
+    if (label_type_ == TAXID) {
+        *taxid = std::stoul(utils::split_string(label, "|")[1]);
+        return true;
+    } else if (label_type_ == GEN_BANK) {
+        std::string acc_version = get_accession_version_from_label(label);
+        auto it = accversion_to_taxid_map_.find(acc_version);
+        if (it == accversion_to_taxid_map_.end()) {
+            return false;
+        }
+        *taxid = it->second;
+        return true;
+    }
+
+    logger->error("Error: Could not get the taxid for label {}", label);
+    exit(1);
+}


where is this called?

get_taxid_from_label will be used in the next PR. I implemented the entire .hpp file in this PR and wanted to add a reasoning about why/how to use the enum LabelType

Signed-off-by: Radu Muntean <[email protected]>

metagraph/src/annotation/taxonomy/tax_classifier.cpp

metagraph/tests/annotation/taxonomy/test_taxonomy.cpp

metagraph/src/annotation/taxonomy/tax_classifier.hpp

Signed-off-by: Radu Muntean <[email protected]>

metagraph/src/annotation/taxonomy/tax_classifier.cpp

metagraph/src/annotation/taxonomy/tax_classifier.hpp

Signed-off-by: Radu Muntean <[email protected]>

heracle requested review from hmusta and karasikov July 30, 2021 19:57

hmusta requested changes Jul 30, 2021

View reviewed changes

hmusta reviewed Jul 30, 2021

View reviewed changes

metagraph/src/annotation/taxonomy/tax_classifier.cpp Outdated Show resolved Hide resolved

hmusta reviewed Jul 30, 2021

View reviewed changes

metagraph/src/annotation/taxonomy/tax_classifier.cpp Outdated Show resolved Hide resolved

heracle force-pushed the mtg_taxclass_3 branch from e3d6c95 to 805735c Compare July 30, 2021 22:13

heracle changed the base branch from mtg_taxclass_2 to master August 3, 2021 20:43

heracle added 8 commits August 3, 2021 22:44

add some small methods that will be further used for tax class

aa34f9d

Signed-off-by: Radu Muntean <[email protected]>

address comments

1ff153f

add some small methods that will be further used for tax class

8f36004

Signed-off-by: Radu Muntean <[email protected]>

add data files used for taxonomic integration and unit tests

2912dec

Signed-off-by: Radu Muntean <[email protected]>

rebase

bb80f5b

taxonomic classification skeleton with the RMQ preprocessing

d9a9861

Signed-off-by: Radu Muntean <[email protected]>

address comments

4fafcb8

Signed-off-by: Radu Muntean <[email protected]>

delete tax test files

76dcb32

Signed-off-by: Radu Muntean <[email protected]>

heracle force-pushed the mtg_taxclass_3 branch from 2852e01 to 76dcb32 Compare August 3, 2021 20:44

karasikov requested changes Aug 4, 2021

View reviewed changes

heracle force-pushed the mtg_taxclass_3 branch from dd1323c to ed55fc4 Compare August 5, 2021 09:56

karasikov reviewed Aug 5, 2021

View reviewed changes

small reformat

db97283

Signed-off-by: Radu Muntean <[email protected]>

heracle force-pushed the mtg_taxclass_3 branch from ed55fc4 to db97283 Compare August 5, 2021 18:39

heracle commented Aug 5, 2021

View reviewed changes

metagraph/src/annotation/taxonomy/tax_classifier.cpp Outdated Show resolved Hide resolved

karasikov requested changes Aug 5, 2021

View reviewed changes

code reformat

f908467

Signed-off-by: Radu Muntean <[email protected]>

heracle force-pushed the mtg_taxclass_3 branch from 82fcdd3 to f908467 Compare August 6, 2021 12:59

karasikov requested changes Aug 8, 2021

View reviewed changes

code reformat

18c0fe0

Signed-off-by: Radu Muntean <[email protected]>

heracle force-pushed the mtg_taxclass_3 branch from 2df6ca8 to 18c0fe0 Compare August 10, 2021 09:01

heracle requested a review from karasikov August 10, 2021 14:18

karasikov requested changes Aug 10, 2021

View reviewed changes

heracle force-pushed the mtg_taxclass_3 branch from 8922b47 to 6bd82bf Compare August 13, 2021 17:47

code reformat

249129e

Signed-off-by: Radu Muntean <[email protected]>

heracle force-pushed the mtg_taxclass_3 branch from 6bd82bf to 249129e Compare August 14, 2021 08:34

heracle requested a review from karasikov August 14, 2021 08:34

karasikov requested changes Aug 16, 2021

View reviewed changes

delete unimplemented headers

ff33e52

Signed-off-by: Radu Muntean <[email protected]>

heracle force-pushed the mtg_taxclass_3 branch from 73ddf85 to ff33e52 Compare August 18, 2021 09:18

heracle requested a review from karasikov August 20, 2021 16:29

karasikov requested changes Aug 31, 2021

View reviewed changes

metagraph/src/annotation/taxonomy/tax_classifier.cpp Outdated Show resolved Hide resolved

metagraph/src/annotation/taxonomy/tax_classifier.hpp Outdated Show resolved Hide resolved

heracle mentioned this pull request Sep 1, 2021

Taxonomic classification #283

Closed

delete get_taxid_from_label method

bc22391

Signed-off-by: Radu Muntean <[email protected]>

heracle force-pushed the mtg_taxclass_3 branch from 584dd59 to bc22391 Compare September 2, 2021 08:14

heracle requested a review from karasikov September 2, 2021 20:02

karasikov merged commit 1609e94 into master Sep 6, 2021

karasikov deleted the mtg_taxclass_3 branch September 6, 2021 08:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

taxonomic classification skeleton with the RMQ preprocessing #346

taxonomic classification skeleton with the RMQ preprocessing #346

heracle commented Jul 30, 2021

heracle commented Jul 30, 2021

heracle commented Aug 5, 2021 •

edited

Loading

karasikov left a comment

heracle commented Aug 5, 2021

karasikov Aug 8, 2021

heracle Aug 10, 2021

karasikov Aug 10, 2021

heracle Aug 13, 2021

karasikov Aug 10, 2021

heracle Aug 13, 2021

	logger->warn("During the tax_tree_filepath {} parsing, {} taxids were not found out of {} total evaluations.",
	logger->warn("During the tax_tree_filepath {} parsing, {} taxids were not found out of {} total evaluations",

taxonomic classification skeleton with the RMQ preprocessing #346

taxonomic classification skeleton with the RMQ preprocessing #346

Conversation

heracle commented Jul 30, 2021

heracle commented Jul 30, 2021

heracle commented Aug 5, 2021 • edited Loading

karasikov left a comment

Choose a reason for hiding this comment

heracle commented Aug 5, 2021

karasikov Aug 8, 2021

Choose a reason for hiding this comment

heracle Aug 10, 2021

Choose a reason for hiding this comment

karasikov Aug 10, 2021

Choose a reason for hiding this comment

heracle Aug 13, 2021

Choose a reason for hiding this comment

karasikov Aug 10, 2021

Choose a reason for hiding this comment

heracle Aug 13, 2021

Choose a reason for hiding this comment

heracle commented Aug 5, 2021 •

edited

Loading