-
Notifications
You must be signed in to change notification settings - Fork 82
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
Updated TEve Event Display to plot the new KKTraj options #900
Conversation
Hi @sophiemiddleton,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. ⌛ The following tests have been triggered for b202857: build (Build queue is empty) |
☀️ The build tests passed at b202857.
N.B. These results were obtained from a build of this Pull Request at b202857 after being merged into the base branch at d88a7ac. For more information, please check the job page here. |
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.
Some questions and suggestions.
using LHPT = KinKal::PiecewiseTrajectory<KinKal::LoopHelix>; | ||
using CHPT = KinKal::PiecewiseTrajectory<KinKal::CentralHelix>; | ||
using KLPT = KinKal::PiecewiseTrajectory<KinKal::KinematicLine>; | ||
template<class KTRAJ> void TEveMu2eDataInterface::AddKinKalTrajectory( std::unique_ptr<KTRAJ> &trajectory, TEveMu2eCustomHelix *line, TEveMu2eCustomHelix *line_twoDXY, TEveMu2eCustomHelix *line_twoDXZ){ |
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 suggest const& trajectory
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.
done
line->SetPoint(0,pointmmTocm(InMu2e.x()), pointmmTocm(InMu2e.y()) , pointmmTocm(InMu2e.z())); | ||
line_twoDXY->SetPoint(0,pointmmTocm(x1), pointmmTocm(y1) , pointmmTocm(z1)); | ||
line_twoDXZ->SetPoint(0,pointmmTocm(x1), pointmmTocm(y1) , pointmmTocm(z1)); | ||
for(double t=t1; t<=t2; t+=0.1) |
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.
Can the time step be configurable? 100ps is ~30mm, which is quite coarse
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 could change by a factor of 10 i.e. 0.01 (or any toher number you want)
I dont think havig it as a FCL parameter is a good idea. We already have such a large amount of parameters
AddKinKalTrajectory<KLPT>(trajectory,line,line_twoDXY,line_twoDXZ); | ||
} | ||
|
||
|
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.
The indenting formatting looks off
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.
fixed
const KalSeedCollection* seedcol = track_list[j]; | ||
colour.push_back(j+3); | ||
if(seedcol!=0){ | ||
for(unsigned int k = 0; k < seedcol->size(); k = k + 20){ |
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.
Why does k advance by 20? it pretty much guarantees you'll only see the 1st track.
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.
set to k++
📝 The HEAD of |
@FNALbuild run build test |
⌛ The following tests have been triggered for b202857: build (Build queue has 1 jobs) |
☀️ The build tests passed at b202857.
N.B. These results were obtained from a build of this Pull Request at b202857 after being merged into the base branch at 1b51ea5. For more information, please check the job page here. |
@sophiemiddleton A gentle reminder that this PR is waiting for you to answer a questions/comments from Dave. |
📝 The HEAD of |
@FNALbuild run build test |
⌛ The following tests have been triggered for b202857: build (Build queue is empty) |
☀️ The build tests passed at b202857.
N.B. These results were obtained from a build of this Pull Request at b202857 after being merged into the base branch at 42696d6. For more information, please check the job page here. |
I also just noticed that there are whitespace errors - so please address those too. If you follow the link beside the whitespace link you will see where the problems are. You need to be on fgz or on VPN to see it. Or you can use curl to download the file when you are logged into a mu2egpvm node. |
I think I addressed the various issues, let me know if not |
@FNALbuild run build test |
⌛ The following tests have been triggered for d73a510: build (Build queue is empty) |
☀️ The build tests passed at d73a510.
N.B. These results were obtained from a build of this Pull Request at d73a510 after being merged into the base branch at 430bd32. For more information, please check the job page here. |
This code allows plotting of the KinKal trajectories as requested by @brownd1978
Note - the REve code is the newest version of the Eve Event Display so any comments on general coding should be provided on that version not this. TEve is maintained as REve currently does not support all the features we developed there.