-
Notifications
You must be signed in to change notification settings - Fork 1
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
Write t3_isFake to the debug output #57
Write t3_isFake to the debug output #57
Conversation
@@ -8,6 +8,8 @@ | |||
#include "ModuleConnectionMap.h" | |||
#include "SDLMath.h" | |||
#include "Event.h" | |||
#include <cppitertools/enumerate.hpp> |
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.
SegmentLinking/TrackLooper#376
I'm not sure we intended to go back
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 I just copied the code from this branch https://github.com/SegmentLinking/TrackLooper/tree/lstod to get a working version. I can refactor the code later to get rid of cppitertools
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.
It would be good to do it now since the dependence is very minimal. It's only needed in two lines:
for (auto &&[ihit, ihitdata] : iter::enumerate(iter::zip(hitidxs, hittypes))) |
for (auto &&[ihit, ihitdata] : iter::enumerate(to_check_duplicate)) |
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'd even suggest to rewrite the git history so that cppitertools doesn't show up in the logs/pack
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'd even suggest to rewrite the git history so that cppitertools doesn't show up in the logs/pack
The last update still has the original commit that adds cppitertools
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 removed the commit with cppitertools
|
||
if (verbose) { | ||
std::cout << " hitidx: " << hitidx << " hittype: " << hittype << std::endl; | ||
if (verbose) |
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 found some places in the codes are exactly the same for before and after the changes, except the formats. Can you don't do the clang-format, so that the codes don't show a lot of git adds and deletes? To help the review process easier? Thank you!
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 reformatted the code so that the styling is consistent, but I'm not sure if it helps with git changes. Please let me know if there's a better way to do it
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 would suggest let's merge this quick, since it's simple functions in the development branch, so that Jade and I can develop from here. Let's not make the formatting change and we can discuss how we want to do it in LST meetings.
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.
As discussed during the Hackaton: please, do not merge this.
183646a
to
7ff7999
Compare
// Sort indices based on 'values' | ||
// auto indices = sort_indices(result_frac); | ||
|
||
// // Reorder 'vec1' and 'vec2' based on the sorted indices |
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.
still, why are those commented lines needed? If those are important, please uncomment them. Otherwise please drop them
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.
These lines sort the sim tracks by match fraction. I think that's not needed for our use case but correct me if I'm wrong
else { | ||
pairs[idx] = frac; | ||
} | ||
// if (std::find(result.begin(), result.end(), idx) == result.end()) |
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.
same
Let's go from the PR #59 |
PR description:
PR validation:
If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
Before submitting your pull requests, make sure you followed this checklist: