-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Remove all the ocurrences of SetCanExtend from TrackAnalyzer #27535
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27535/10911
|
A new Pull Request was created by @hbecerri for master. It involves the following packages: DQM/TrackingMonitor @andrius-k, @kmaeshima, @schneiml, @cmsbuild, @jfernan2, @fioriNTU can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@hbecerri thanks for this PR, however, despite the discussion in #27535 it would be nice to know why the wf was creating eta histograms from first: (31, -15.000000, 9.000000), second: (31, -9.000000, 15.000000) as shown by @fabiocos on #27528 (comment) |
I think this happened exactly because of the fact that SetCanExtend was activated for this profile (while there is no reason for it to be)
What do you mean? It's an era modifier. The phase0 detector had smaller pseudorapidity coverage hence the |eta|<2.5 axis limits, which was extended to |eta|<3 for the new phase1 detector. |
Can tests be started here please? |
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
hold |
Pull request has been put on hold by @jfernan2 |
@mmusich It is also disappointing that you skim my comments |
what does "skim" mean in this context?
for my education, how can you say it is created with 26 bins?
there is no point in extending this particular histogram, I don't follow your logic. |
overlook, but never mind
https://cmssdt.cern.ch/lxr/source/DQM/TrackingMonitor/python/TrackingMonitor_cfi.py#0258
I was talking about Phase0 Pixel plots
I don't say it is logic to extend it but that the code was allowing to extend it in first instance, and it gets actually extended if you allow it: https://tinyurl.com/y4gaqlsg
My point is WHERE they get extended in your code, to avoid problems in the future |
but this is valid only for phase-0 plots and not for phase-1 plots, where the era modifier prevails.
right, but I am still not getting your point. Sorry I must be very dense :(
but isn't it true that any plot can be extended?
They get extended all over the place where it makes sense, like in time or LS series: (see e.g. https://github.com/cms-sw/cmssw/blob/master/DQM/TrackingMonitor/src/TrackingMonitor.cc#L348 for an example). |
@jfernan2 @mmusich @hbecerri this PR is indeed addressing an issue that is worthwhile to investigate. But in practice #27531 is solving a problem that has broken a pre-release making it unusable, while the other instances of SetCanExtend are not for what we can see. So I consider solving the former issue as urgent. Given that the discussion on this PR might require a bit more time, I will move forward with merging #27531, this PR can just be rebased on that, and possibly update it in case |
OK, provided that @hbecerri is fine in rebasing. Frankly I don't understand why the due investigations on the issue are a reason enough to hold this PR, which is basically doing the same thing as #27531 (which is already DQM signed) just more cleanly. |
@mmusich the line you point only allows it CAN be extended, and it is a LS based plot which makes sense since you don't know the limits a priori, but not the ones we are talking about which have a known range from the beginning. And not, I am not reviewing PRs based on the submitter but on the urgency. Just to avoid disappointments to you in the future ;-) |
Now I finally see what you mean! I think it's ROOT that eventually decides where to put the boundary, could it be that thee are some nans propagating here? |
This is in particular is wf 4.53 but you can see it is happening in half of the workflows, those with a non zero number in: |
Hi @jfernan2 so I re-run step3 of wf 4.53 by adding these changes: diff --git a/DQM/TrackingMonitor/src/TrackAnalyzer.cc b/DQM/TrackingMonitor/src/TrackAnalyzer.cc
index f1abe45ff75..67e628e20fd 100644
--- a/DQM/TrackingMonitor/src/TrackAnalyzer.cc
+++ b/DQM/TrackingMonitor/src/TrackAnalyzer.cc
@@ -1287,6 +1287,9 @@ void TrackAnalyzer::analyze(const edm::Event& iEvent, const edm::EventSetup& iSe
if (Folder == "Tr") {
DistanceOfClosestApproachToBSdz->Fill(track.dz(bs.position()));
+ std::cout << "====================================================================="<< std::endl;
+ std::cout << "track.eta():" << track.eta() << " track.dxy(bs.position())):" << track.dxy(bs.position()) << std::endl;
+ std::cout<< "DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmax(): " << DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmax () << " | DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmin(): " << DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmin () << std::endl;
DistanceOfClosestApproachToBSVsEta->Fill(track.eta(), track.dxy(bs.position()));
} and I identified two transitions where the x-axis range got updated: =====================================================================
track.eta():2.50979 track.dxy(bs.position())):0.0306698
DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmax(): 2.5 | DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmin(): -2.5
=====================================================================
track.eta():2.34284 track.dxy(bs.position())):-0.0247759
DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmax(): 7.5 | DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmin(): -2.5
===================================================================== and: =====================================================================
track.eta():-2.50028 track.dxy(bs.position())):0.0215483
DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmax(): 7.5 | DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmin(): -2.5
=====================================================================
track.eta():-2.22177 track.dxy(bs.position())):-0.0364012
DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmax(): 7.5 | DistanceOfClosestApproachToBSVsEta->getTH1()->GetXaxis()->GetXmin(): -12.5
===================================================================== so it seems that indeed ROOT automatically updated the range (by some internal heuristic I guess) when we hit a track with a pseudo-rapidity very close to the boundary eta = 2.5 (which is the correct one for a phase-0 pixel detector workflow such as 4.53 (2012B)). |
ok @mmusich that explains why the phi plots are not affected: there are no boundaries there... |
unhold |
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This PR removes all the occurrences of SetCanExtend from TrackAnalyzer.cc. Solves the issue discussed in #27531
PR validation:
tested using runTheMatrix.py -l 10024