-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/100 crawling mcp solver #101
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
base: main
Are you sure you want to change the base?
Conversation
…enome-downsampler into feature/100-crawling-mcp-solver
mytkom
left a comment
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.
I will do another review of algorithm/test code, I did some obvious comments for now to get these shallow things done.
migoox
left a comment
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.
For now mostly XRay related comments, I will look into the cpp code later
| endif() | ||
|
|
||
| if(NOT CMAKE_BUILD_TYPE) | ||
| set(CMAKE_BUILD_TYPE Release CACHE STRING "Choose the type of build." FORCE) |
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.
Since we are moving to presets I think it's not necessary
mytkom
left a comment
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.
I've given some ideas for improving readability of your code. Please, do not trust them too much, think of them and answer if in your opinion it is: a) possible to implement it this way b) wouldn't worsen the performance much.
| } | ||
|
|
||
| // TODO find better name | ||
| struct HelperRead |
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 struct will be unnecessary if we add those operator methods to the Read struct, but we would probably also need to compare the start index (and only if this is equal, we compare end indices), which will add additional overhead to your code.
So idk if this would be a positive change, we should discuss it with other team members---if it would be useful for them to have such operators available.
| is_first_reads.reserve(size); | ||
| } | ||
|
|
||
| void printReads() |
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.
Should be snake case, and be named differently. You are not printing reads, but the scheme of their mapping to the reference genome. This print would also lose sense if the reference genome length were longer than tens of bp. I wonder if we shouldn't add a new verbosity level for such methods, because -v flag (logging::DEBUG) can be quite useful for the user now, but adding your prints to it would totally destroy the readability of prints for real-world usage scenarios (~30k bp reference genome length).
I also wonder if it wouldn't be nice to create, let's call it, a decorator class, which will wrap the SOAPairedReads object and provide operator<< overload, which then could be used with LOG_WITH_LEVEL macro. I am not sure about the names, but it will look something like that:
LOG_WITH_LEVEL(logging::TEST) << BinarySchemeVisualizer(soa_paired_reads_object);Then, on the level of the algorithm's code, you can see everything:
- What is the verbosity level?
- What object is being printed?
- What is the format of the print?
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.
I agree. We should avoid std::cout and friends in favor of logger, as we can control the verbosity levels via v flag as @mytkom explained
| } | ||
|
|
||
| inline void push_back(const Read& read) override { | ||
| inline void push_back(const Read& read) override |
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.
According to cpp-reference:
A function defined entirely inside a class/struct/union definition, whether it's a member function or a non-member friend function, is implicitly an inline function unless it is attached to a named module(since C++20).
so these inline specifiers are redundant in the SOAPairedReads
| is_first_reads.reserve(size); | ||
| } | ||
|
|
||
| void printReads() |
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.
I agree. We should avoid std::cout and friends in favor of logger, as we can control the verbosity levels via v flag as @mytkom explained
|
|
||
| // Structure of Arrays (SoA) implementation | ||
| struct SOAPairedReads : PairedReads { | ||
| struct SOAPairedReads : PairedReads |
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.
I think that we can use the public inheritance here to allow polimorphic calls
No description provided.