Add track cluster impact angle plot #21023
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
A new Pull Request was created by @ssilvado for master. It involves the following packages: DQM/SiPixelPhase1TrackClusters @kmaeshima, @cmsbuild, @vanbesien, @vazzolini, @dmitrijus can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@dmitrijus I asked Sheila to introduce herself, she is a brand new developer of TkDQM, both Online and Offline |
|
@dmitrijus I had introduced myself to the DQM core team. I am Sheila Amaral post-doc at Purdue University Northwest and I am starting to work as a developer for Tracker DQM (Online and Offline). As Francesco asked me, I am working on the implementation of the Impact angle on DQM. So, I have it done on my GitHub area, and my username is @ssilvado. |
|
+1 |
|
The tests are being triggered in jenkins. |
cmsbuild
added
dqm-approved
fully-signed
and removed
dqm-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
Pull request #21023 was updated. @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @vanbesien can you please check and sign again. |
|
@davidlange6 @VinInn @mtosi how do we proceed here? I can ask my technical student to make the Pixel DQM @ HLT completely independent by what used in Tracker DPG monitoring. Of course the code will stay as "property" of the Trigger group, other (more direct) solution would be to switch off the monitoring at HLT (who is looking to those plots?) |
|
I think doing these three things will make for a robust solution
1) remove unused or duplicated monitoring
2) replace copied configs with an import statement to share code if they need to remain in sync
3) Fix the fundamentally wrong design: separate the [make the c++ list (link 1) the set of possible monitored quantities and the python list (example is link 2) the set of selected monitored quantities for the individual module
link 1 https://github.com/ssilvado/cmssw/blob/d9fb686e4ffdce72512a70ec72b67cc6a6d5c1e7/DQM/SiPixelPhase1TrackClusters/src/SiPixelPhase1TrackClusters.cc#L33
link 2
https://github.com/ssilvado/cmssw/blob/d9fb686e4ffdce72512a70ec72b67cc6a6d5c1e7/DQM/SiPixelPhase1TrackClusters/python/SiPixelPhase1TrackClusters_cfi.py#L389
… On Nov 7, 2017, at 9:33 AM, fioriNTU ***@***.***> wrote:
@davidlange6 @VinInn @mtosi how do we proceed here? I can ask my technical student to make the Pixel DQM @ HLT completely independent by what used in Tracker DPG monitoring. Of course the code will stay as "property" of the Trigger group, other (more direct) solution would be to switch off the monitoring at HLT (who is looking to those plots?)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
the tracker-TSG contact checks those plots ~daily !
as said/written many times
I'mnot in favour of having dedicated code, it does not make any sense at all
…On Tue, Nov 7, 2017 at 9:33 AM, fioriNTU ***@***.***> wrote:
@davidlange6 <https://github.com/davidlange6> @VinInn
<https://github.com/vininn> @mtosi <https://github.com/mtosi> how do we
proceed here? I can ask my technical student to make the Pixel DQM @ HLT
completely independent by what used in Tracker DPG monitoring. Of course
the code will stay as "property" of the Trigger group, other (more direct)
solution would be to switch off the monitoring at HLT (who is looking to
those plots?)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21023 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEt58znD8qiAFbr0ZGuR7eLHj3Q9MvVVks5s0BW-gaJpZM4QGvHK>
.
|
|
@davidlange6 these are my thoughts about your points:
@mtosi, again I do not understand why you are against this? Having the code like it is, is a problem, for both of us, so what better solution you propose? |
|
On Nov 7, 2017, at 10:45 AM, fioriNTU ***@***.***> wrote:
@davidlange6 these are my thoughts about your points:
• This is more a matter of optimization, and we are working on it (with lower priority), however if HLT uses our plugin it is impossible to remove the many plots instantiated in the c++ code and not used in HLT monitoring ... we have to go to a separate plugin, let me say again that this is a super easy implementation
nominally the offline and online parts of the tracker ought to agree on whats important to monitor for their detector (even if the two groups might look at different things as high or low priority..)
but thats my third point. just because the code _can_ make a plot, doesn't mean that it should be _required_ to make that plot for all modules instances (but that is basically the current design)
• I think anything is to stay in sync if we decide to go for a separate plugin (the risk that bugs are propagated to the HLT plugin are close to zero)
I disagree.
• This is in the TkDQM plans, @VinInn changed the structure of the code to add few plots which could have been added in a more natural way. With this PR #21048 we are able to add inner and outer ladders histograms without adding a single line in c++ and maintaning the basic structure of Phase 1 DQM
its nothing to do with changing or not changing c++... its having to change c++ and python in sync. Break that dependency and your life will be better.
… @mtosi, again I do not understand why you are against this? Having the code like it is, is a problem, for both of us, so what better solution you propose?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
" but thats my third point. just because the code can make a plot, doesn't mean that it should be required to make that plot for all modules instances (but that is basically the current design)" this is not true, with a "enabled=False" in the cfg histograms are not booked. I think the design of the code is "smart" compared to what we use in Strip and/or Tracking DQM, it reduces a lot the code needed to monitor the different parts of the detector (Layers, Shells, Ladders, Blades, Modules, FEDs, ROCs ... etc). For sure we have to pay a prize for it, and having the c++ and python dependent (only on the order of PSets) is a fairly low price to pay in my opinion. The code is still clean and super readable (even if many, many newcomers have edited it), I would prefer to keep as it is. |
|
On Nov 7, 2017, at 11:04 AM, fioriNTU ***@***.***> wrote:
@davidlange6
" but thats my third point. just because the code can make a plot, doesn't mean that it should be required to make that plot for all modules instances (but that is basically the current design)"
this is not true, with a "enabled=False" in the cfg histograms are not booked. I think the design of the code is "smart" compared to what we use in Strip and/or Tracking DQM, it reduces a lot the code needed to monitor the different parts of the detector (Layers, Shells, Ladders, Blades, Modules, FEDs, ROCs ... etc). For sure we have to pay a prize for it, and having the c++ and python dependent (only on the order of PSets) is a fairly low price to pay in my opinion. The code is still clean and super readable (even if many, many newcomers have edited it), I would prefer to keep as it is.
'
regardless, having a set of things in c++ and and set of things in python that _must_be the same is a big design problem. I doubt its a complicated change
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@davidlange6 just a small clarification, then maybe we should meet (informally) to close this thread. then the python has only to respect the order (not really to contain the same strings), so in the cfi we have at first the PSet for charge, then size ... etc. This is what usually we pay. Then let me point out that this kind of issue happens ONLY if we add a new quantity to monitor (if we want to add or remove plots with existing quantities it requires only deleting or adding one line of python), since this has been the first year with this new detector I believe that cases in which we want to add new quantities to the monitoring will be really rare. Said that, I would be more than happy to discuss with you how to improve the code if you already have ideas. |
|
hi @fioriNTU -my suggestion is to expand on this [not so well tested, but in theory one just has to fill more if-else statements in the constructor to cover all of the possible cases] this will break any strong ordering requirement between c++ and python as well as any requirement that all possible psets are needed in each module instance. The cost is a set of string->int lookups in the constructor and ifs to check against the validity of each histogram (which likely can be done better) |
|
@davidlange6 thanks for the feedback, however I would like to think about it more. In this way you remove the order dependency but, as you point out, we have to add a lot more if statements, and we have to change the each plugin ... who knows how many bugs we can introduce in this. I am not 100% sure if this will make our life better. I plan to work on this after the run is over, will be back to you in few weeks. |
|
On Nov 8, 2017, at 3:58 PM, fioriNTU ***@***.***> wrote:
@davidlange6 thanks for the feedback, however I would like to think about it more. In this way you remove the order dependency but, as you point out, we have to add a lot more if statements, and we have to change the each plugin ... who knows how many bugs we can introduce in this. I am not 100% sure if this will make our life better. I plan to work on this after the run is over, will be back to you in few weeks.
indeed, this functionality could all be moved into one spot if things were hidden a bit more in the base class. Given the number of consumers that is probably a good idea.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Your PR is unmergeable. Please have a look and possibly rebase it. |
|
@ssilvado what should we do with this PR, that in any case should be as a minimum be rebased? |
|
Hi @ssilvado , just close this PR for the moment to remove it from the github list - We should think and discuss all those issues calmy (maybe first remember them ) in a tkDQM meeting - Thanks |
|
@ssilvado please close this PR, at this point for sure is not merge-able anymore. We will cook a new one, actually these plots have been mentioned in one of the last DPG meetings, so it is worth to add them. |
Added the track cluster impact angle plot