PR following PR #326 - removed obsolete hit class [Needs testing] by drasal · Pull Request #332 · tkLayout/tkLayout

drasal · 2017-03-23T18:17:51Z

Code made fully compatible with new Track & Hit classes -> needs testing on CMS Ph2 layouts!

Added missing functionality from hit class to TrackNew & HitNew classes.
Namely, added HitPassiveType which may be of BeamPipe, Service, Support or IP -> necessary for inactive hits in Hit class.
Additionally, added tracking volumes variables to Hit class.
Finally, Analyzer code adjusted to new smart pointer policy used in TrackNew class & HitNew class.
HitNew & TrackNew classes then renamed to regular Track & Hit classes. hit.cpp & hit.hh completely removed.

* Added missing functionality from hit class to TrackNew & HitNew classes. * Namely, added HitPassiveType which may be of BeamPipe, Service, Support or IP -> necessary for inactive hits in Hit class. * Additionally, added tracking volumes variables to Hit class. * Finally, Analyzer code adjusted to new smart pointer policy used in TrackNew class & HitNew class. * HitNew & TrackNew classes then renamed to regular Track & Hit classes. hit.cpp & hit.hh completely removed.

drasal · 2017-03-24T09:58:29Z

Visual testing of OT365_200_IT4024 geometry taken as a reference doesn't show any discrepancies ...

drasal · 2017-03-28T12:03:23Z

Merge with Stefano's changes done -> works fine!!!

alkemyst · 2017-03-31T12:08:55Z

Hi,
I am testing the code now with the following options:
tklayout --performance -d -a -N 3000 -n 50000 -T -R -w geometries/CMS_Phase2/OT613_200_IT4025.cfg
and I spotted the following changes:

The "Material (outer)" tab - in the section "Nuclear interactions" the interactions, the pixel hits are now taken into account. We can take this as a feature change.
regression Still in the "Material" tab (both outer and pixel) - in the section "Services details (Full volume)" the material table disappeared and the plot disappeared too 👎
regression? in the "Material (total)" section the total material amount changed drastically with a large increase of the component named "Services and Supports under Pixel Tracking Volume" from 0.00015 X₀ to 0.048 X₀ and the component Services and Supports between Pixel and Outer Tracking Volumes from 0.06953 X₀ to 0.15834 X₀
Looking in the details (full layout material : components) the change is entirely due to "Services under Pixel Tracking Volume" and "Services and supports in interstice"

BEFORE:

AFTER:

drasal · 2017-03-31T12:15:40Z

Stefano, can you, please point me out to the web-page? It's very hard to see anything from these plots...

alkemyst · 2017-03-31T12:23:40Z

OK I am rebuilding it now and putting it online.

alkemyst · 2017-03-31T12:57:51Z

Here it is:
http://mersi.web.cern.ch/mersi/layouts/testing_pr326/Material%20(total).html

drasal · 2017-03-31T13:08:00Z

So, it looks like Services were lost somewhere on the way ... The rest is OK, so it should be a trivial bug. Do you have an idea/hint, where it could be? I may have overlooked something ...

alkemyst · 2017-03-31T13:16:06Z

Do you mean that the new material budget estimate is more accurate? That sounds like baaaaad news...

drasal · 2017-03-31T13:20:58Z

No, I mean that in my code, I may have "forgotten" or more precisely overlooked adding the services hits. That's why I'd need to get navigated to the part of the code, where it's done. Would you have hints?

alkemyst · 2017-03-31T13:29:23Z

Did you notice that your code gives *higher* material budget than the expected behaviour? Please check previously linked page [0] against reference [1] [0] http://mersi.web.cern.ch/mersi/layouts/testing_pr326/Material%20(total).html [1] http://cms-tklayout.web.cern.ch/cms-tklayout/layouts/repository-git-dev_gabie/OT613_200_IT4025/Material%20(total).html

drasal · 2017-03-31T13:35:04Z

Ah, I understand what you mean. Nevertheless, if you look at MB of pixel or outer only part, everything is correct except for the services, so let's fix that first. Then we may have a look on total MB... I don't believe that your "old" SW was giving wrong results. So, don't worry!

alkemyst · 2017-03-31T13:38:14Z

Just as another pointer to the issue: the 2D material maps of the two branches (under "material outer") are identical.

drasal · 2017-03-31T16:13:16Z

Dear Stefano,
I'm afraid that you may have had a BUG in the older version. If you sum-up in the older version numbers at Material (pixel) & material(outer) - categories details, you will get: 0.4952. If you do the same (without beam-pipe) on Material(total) you get: 0.44837. If you do the same calculations in the new version, you will get almost the same numbers (due to averaging across 2 histograms and summing them up versus 1 histogram they may slightly differ).

As for the missing histogram, there was a small bug in my SW, which caused the non-existing service histograms, but that's fixed by now.

To understand the details, please, have a look at Analyzer::analyzeInactiveSurfaces methods. You will find out quickly, why the results now differ compared to the older version. To be sure that everything is correct, you may want to go again through the procedure of how the Total material budget is being calculated, I haven't written that code, so it's very difficult for me to understand all the details...

Please, let me know, whether this makes sense ...

ghugo83 · 2017-04-03T11:57:12Z

The MB results aforementioned mentioned in [0] http://mersi.web.cern.ch/mersi/layouts/testing_pr326/Material%20(total).html do not physically make sense :

Services and Supports under Pixel Tracking Volume : huge orange part.
In reality, there is no or nearly no material under the Pixel tracking volume though.
This cannot make sense.
Services and Supports between Pixel and Outer Tracking Volumes : one can see the green part is going on after eta > 2.8.
This is not possible, it should be MB = 0. for eta > 2.8 by definition.

ghugo83 · 2017-04-03T11:58:39Z

I am 100% sure on the results obtained with tkLayout present code for the following reasons :

careful cross-checks;
obtained same Phase 2 tracking volume MB with independent hacks in CMSSW code.

ghugo83 · 2017-04-03T12:04:23Z

First : I think there is an error in the makefile.
Stefano, I think you did not compile with this exact branch's code ?

Though, it is true that there are different results for the categories 'Services and Supports under Pixel Tracking Volume' and 'Services and Supports between Pixel and Outer Tracking Volumes' between Zbynek's code VERSUS our official code.
Though, Zbynek's results do not show any 'obvious' unphysical mistake.
But he gets around double MB in the 2 categories : 'Services and Supports under Pixel Tracking Volume' and 'Services and Supports between Pixel and Outer Tracking Volumes'.

Results with Zbynek's code :
http://ghugo.web.cern.ch/ghugo/layouts/2016/OT613_200_IT4025_Zbynek/Material%20%28total%29.html
Reminder of our official results :
http://cms-tklayout.web.cern.ch/cms-tklayout/layouts/repository-git-dev_gabie/OT613_200_IT4025/Material%20%28total%29.html

ghugo83 · 2017-04-03T12:08:17Z

Before and after this PR (edit of Stefano's comment) :
BEFORE :

AFTER :

ghugo83 · 2017-04-03T12:13:04Z

After some investigation, it turns out the issue is in Zbynek's code indeed.

Relevant boundaries for a track at eta = 2.00067 :
firstActiveHitPixelDistance = 115.286
lastActiveHitPixelDistance = 540.529
firstActiveHitOuterDistance = 897.551
lastActiveHitOuterDistance = 2770.95

In the 'Services and Supports between Pixel and Outer Tracking Volumes' category, whatever is summed up at this eta is :
eta = 2.00067 RL = 0.0574978
distance = 115.286

eta = 2.00067 RL = 0.013162
distance = 257.241

eta = 2.00067 RL = 0.013162
distance = 261.391

eta = 2.00067 RL = 0.0131477
distance = 333.75

eta = 2.00067 RL = 0.0131477
distance = 540.529

eta = 2.00067 RL = 0.00440801
distance = 552.535

eta = 2.00067 RL = 0.0170539
distance = 628.457

eta = 2.00067 RL = 0.0150763
distance = 651.278

eta = 2.00067 RL = 0.0166015
distance = 669.492

eta = 2.00067 RL = 0.0333028
distance = 711.512

eta = 2.00067 RL = 0.0167358
distance = 796.219

One can cleary see that materials that do not belong to the 'Services and Supports between Pixel and Outer Tracking Volumes' category are wrongly added.

ghugo83 · 2017-04-03T12:14:26Z

The error comes from the fact that in Hit constructor, the tracking volumes bools are not initialized, and hence the default value for a bool = true in C++ is taken.

ghugo83 · 2017-04-03T12:15:56Z

+    m_isPixel          = false;
+    m_resolutionRPhi   = 0;
+    m_resolutionZ      = 0;
+


ERROR !
Should add here :

m_isPixelIntersticeVol= false;

m_isPixelTrackingVol = false;

m_isIntersticeVol = false;

m_isOuterTrackingVol = false;

m_isTotalTrackingVol = false;

Than, I have to apologize for this bug ...

ghugo83 · 2017-04-03T12:17:44Z

With the proposed fix ahead, I get the exact same Material Budget results in this branch as the official tkLayout's results.
CQFD ;p

drasal · 2017-04-03T12:51:16Z

Gabi, thanks very much for the full cross-check and finding the bug. It was very difficult for me to spot such a discrepancy, as I don't work with the CMS Ph2 geometry at all...

…uctor.

ghugo83 · 2017-04-03T15:37:34Z

You're welcome :)

Also, very small thing this time, but let's do it : should update the Makefile as well (there is no TrackNew Hitnew)

drasal · 2017-04-03T15:39:03Z

Gabi, Makefile is not my cup of tee, can you, please do it?

Pico PR : removed HitNew and TrackNew classes from Makefile

drasal · 2017-04-06T14:04:14Z

Gabi, Stefano,
are the results already fine? I would sort out the conflicts ...

ghugo83 · 2017-04-06T14:28:57Z

Hmm the Tracking Material Budget matches exactly the official layouts after the bug fix.
For the rest I haven't looked yet ;)

drasal · 2017-04-07T14:09:25Z

Gabi, can you, please have a look and let me know?

alkemyst · 2017-06-09T15:25:59Z

Hi all,
is this PR still relevant, or did this development make it to the main code via another branch?

'S

drasal · 2017-06-12T07:51:11Z

Hi Stefano,

I'm afraid it's still relevant. It removed completely the old hit class and changed it to Hit & Track classes, without need for HitNew & TrackNew. But I couldn't test that everything is in shape with CMS layout, that's why I relied on that on Gabi...

ghugo83 · 2017-06-12T07:56:01Z

Yep I had had a look at that time, but from what I remember did not fully check against all regressions.
Will have a look!

drasal · 2017-06-12T07:58:16Z

Hi Gabi,

that would be great! Let me know, if you find some issues, so that I can fix it. FCC week is over, so I'm a little more relaxed now ...

ghugo83 · 2017-12-01T10:55:19Z

Thanks Zbynek for this PR!
Apart from the regression in Material Budget, there were 2 minor regressions:

Local reso mean and std were not displayed anymore on geometry tables.
In OT tab, 'Hits occupancy & track efficiency for hadrons' plot was not displaying (OT) results anymore but (OT + IT) results.

There is now no regression with this PR anymore (fully cross-checked).

I have solved all merging conflicts and reopened the PR at #409 , on top of the latest changes from April until now.
It will be merged on Monday to dev_gabie :)

drasal added 2 commits March 24, 2017 11:05

Update comments in Track class

1a8ba1a

Merged official with dev_gabie branch -> working fine!

c05f781

alkemyst self-requested a review March 31, 2017 11:11

alkemyst removed their request for review March 31, 2017 12:10

For several services forgotten to set isPixel variable

00b00da

ghugo83 reviewed Apr 3, 2017

View reviewed changes

Fixed bug found by Gabi, several variables not set to false in constr…

497c59f

…uctor.

ghugo83 and others added 2 commits April 4, 2017 10:06

Removed HitNew and TrackNew classes from Makefile

eb30de9

Merge pull request #23 from ghugo83/nohit

70b7225

Pico PR : removed HitNew and TrackNew classes from Makefile

ghugo83 mentioned this pull request Dec 1, 2017

Merge PR#332 from Zbynek: renaming HitNew -> Hit, TrackNew -> Track #409

Merged

ghugo83 closed this Dec 1, 2017

Uh oh!

Conversation

drasal commented Mar 23, 2017

Uh oh!

drasal commented Mar 24, 2017

Uh oh!

drasal commented Mar 28, 2017

Uh oh!

alkemyst commented Mar 31, 2017

Uh oh!

drasal commented Mar 31, 2017

Uh oh!

alkemyst commented Mar 31, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alkemyst commented Mar 31, 2017

Uh oh!

drasal commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alkemyst commented Mar 31, 2017 via email

Uh oh!

drasal commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alkemyst commented Mar 31, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drasal commented Mar 31, 2017

Uh oh!

alkemyst commented Mar 31, 2017 via email

Uh oh!

drasal commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghugo83 commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghugo83 commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghugo83 commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghugo83 commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghugo83 commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghugo83 commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghugo83 Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drasal Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

ghugo83 commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drasal commented Apr 3, 2017

Uh oh!

ghugo83 commented Apr 3, 2017

Uh oh!

drasal commented Apr 3, 2017

Uh oh!

drasal commented Apr 6, 2017

Uh oh!

ghugo83 commented Apr 6, 2017

Uh oh!

drasal commented Apr 7, 2017

Uh oh!

alkemyst commented Jun 9, 2017

Uh oh!

drasal commented Jun 12, 2017

Uh oh!

ghugo83 commented Jun 12, 2017

alkemyst commented Mar 31, 2017 via email •

edited

Loading

drasal commented Mar 31, 2017 •

edited

Loading

drasal commented Mar 31, 2017 •

edited

Loading

alkemyst commented Mar 31, 2017 via email •

edited

Loading

drasal commented Mar 31, 2017 •

edited

Loading

ghugo83 commented Apr 3, 2017 •

edited

Loading

ghugo83 commented Apr 3, 2017 •

edited

Loading

ghugo83 commented Apr 3, 2017 •

edited

Loading

ghugo83 commented Apr 3, 2017 •

edited

Loading

ghugo83 commented Apr 3, 2017 •

edited

Loading

ghugo83 commented Apr 3, 2017 •

edited

Loading

ghugo83 Apr 3, 2017 •

edited

Loading

ghugo83 commented Apr 3, 2017 •

edited

Loading

drasal commented Jun 12, 2017 •

edited

Loading

ghugo83 commented Dec 1, 2017 •

edited

Loading