Skip to content

Energy threshold update for new MTD geometry #31654

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.

Sign up for GitHub

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

Closed
gsorrentino18 wants to merge 7 commits into from
Closed

Energy threshold update for new MTD geometry #31654

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2390bc2
Modify threshold for the new MTD ETL topology
gsorrentino18 Sep 23, 2020
ce31890
Eliminate redundant if cycles
gsorrentino18 Oct 2, 2020
7f999d4
Minor fixes
gsorrentino18 Oct 2, 2020
506a52f
code-check and code-format update
gsorrentino18 Oct 2, 2020
1814869
minor fixes
gsorrentino18 Oct 2, 2020
cb56598
Fixing hardcoded topologymode
gsorrentino18 Oct 5, 2020
98c389f
commit d599c6e7a72ae9399d67f12cda4c03f52f7bf6f3
gsorrentino18 Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions RecoLocalFastTime/FTLCommonAlgos/plugins/MTDRecHitAlgo.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,16 @@
#include "RecoLocalFastTime/Records/interface/MTDTimeCalibRecord.h"
#include "RecoLocalFastTime/FTLCommonAlgos/interface/MTDTimeCalib.h"

#include "Geometry/MTDNumberingBuilder/interface/MTDTopology.h"
#include "Geometry/MTDCommonData/interface/MTDTopologyMode.h"
#include <iostream>
Copy link
Contributor

@kpedro88 kpedro88 Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this include should not be needed, because cout is not allowed.


class MTDRecHitAlgo : public MTDRecHitAlgoBase {
public:
/// Constructor
MTDRecHitAlgo(const edm::ParameterSet& conf, edm::ConsumesCollector& sumes)
: MTDRecHitAlgoBase(conf, sumes),
thresholdToKeep_(conf.getParameter<double>("thresholdToKeep")),
thresholdToKeep_(conf.getParameter<std::vector<double>>("thresholdToKeep")),
calibration_(conf.getParameter<double>("calibrationConstant")) {}

/// Destructor
Expand All @@ -22,14 +26,19 @@ class MTDRecHitAlgo : public MTDRecHitAlgoBase {
FTLRecHit makeRecHit(const FTLUncalibratedRecHit& uRecHit, uint32_t& flags) const final;

private:
double thresholdToKeep_, calibration_;
std::vector<double> thresholdToKeep_;
double calibration_;
const MTDTimeCalib* time_calib_;
const MTDTopology* topology_;
};

void MTDRecHitAlgo::getEventSetup(const edm::EventSetup& es) {
edm::ESHandle<MTDTimeCalib> pTC;
es.get<MTDTimeCalibRecord>().get("MTDTimeCalib", pTC);
time_calib_ = pTC.product();
edm::ESHandle<MTDTopology> topologyHandle;
es.get<MTDTopologyRcd>().get(topologyHandle);
topology_ = topologyHandle.product();
}

FTLRecHit MTDRecHitAlgo::makeRecHit(const FTLUncalibratedRecHit& uRecHit, uint32_t& flags) const {
Expand All @@ -39,6 +48,11 @@ FTLRecHit MTDRecHitAlgo::makeRecHit(const FTLUncalibratedRecHit& uRecHit, uint32
float energy = 0.;
float time = 0.;

// MTD topology
unsigned int index_topology = 0; //1Disks geometry
Copy link
Contributor

@slava77 slava77 Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like this index can be computed in getEventSetup, no need to re-derive it for every hit

if (topology_->getMTDTopologyMode() > static_cast<int>(MTDTopologyMode::Mode::barphiflat))
Copy link
Contributor

@slava77 slava77 Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not == btlv1etlv4 (with whatever appropriate casts). I do not see any design constraints that would prevent defining another value in MTDTopologyMode::Mode, which would correspond again to index 0.

Please remind me how do we get barphiflat vs btlv1etlv4 vs some other value of getMTDTopologyMode?
It looks like a property of the full detector (mixing of different values is not possible).
Ideally, then the threshold should come from ES in a coherent way.
Having both values in the same configuration is a bit confusing, because only one can possibly be applicable.
The alternative to using ES is an appropriate Era modifier.
Only if the options above are rejected, I'd consider injecting multiple incompatible caibration/configuration parameters into the same module.

Copy link
Contributor

@fabiocos fabiocos Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slava77 up to barphiflat (i.e. 4) this enum was used to distinguish various BTL scenarios, now all gone but the last one. I use the same enum now to distinguish ETL scenarios, btlv2etlv4 is the first implementation in D53, but another one is about to be stored as well. This value comes with the MTD topology object, and should be considered as a temporary tool to identify within the code which geometry is being processed, as soon as the choice will be done this will not be needed any more.

In the specific case of this parameter, there is a change in thickness of the silicon detector in the change of scenarios that justify this move, and this change will not be a function of the new scenarios (i.e. LGAD thickness should be fixed now regardless of the geometrical arrangement of modules on the discs' faces). So for what we know now, this selection is ok. Your suggestion, if I understand correctly, would be to create an EStype object that provides these parameters as a function of the topology mode, do I understand it correctly? @casarsa @gsorrentino18 do you see other parameters that need to be tuned?

Copy link
Contributor

@slava77 slava77 Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

up to barphiflat (i.e. 4) this enum was used to distinguish various BTL scenarios, now all gone but the last one. I use the same enum now to distinguish ETL scenarios, btlv2etlv4 is the first implementation in D53, but another one is about to be stored as well.

I'm not sure if this historical precedent can suggest that in the future any new enum will also be about ETL 2-disk cases; that's why I think that selection of values explicitly is more appropriate than a range comparison >.

Your suggestion, if I understand correctly, would be to create an EStype object that provides these parameters as a function of the topology mode, do I understand it correctly?

That would be an ideal case; it's probably not much complicated because at least in this case the related data is already handled via ES.
An era modifier is another option.

Copy link
Contributor

@casarsa casarsa Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@casarsa @gsorrentino18 do you see other parameters that need to be tuned?

@fabiocos In principle, yes, the calibration constant (the ETL energy is converted into MIP units in the simulation and the MIP peak value depends on the thickness of the sensor), but we are planning on tackling that in another dedicated PR that modifies also the simulation code.
As for this PR, the calibration constant is an internal parameter (and currently kind of arbitrary) and as long as its value is consistent between simulation and reconstruction the result is independent of its actual value.

Copy link
Contributor Author

@gsorrentino18 gsorrentino18 Oct 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be an ideal case; it's probably not much complicated because at least in this case the related data is already handled via ES.
An era modifier is another option.

Following your suggestions, we are indeed thinking about implementing an Era Modifier to provide the parameters as a function of the geometry.

Copy link
Contributor

@fabiocos fabiocos Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpedro88 @slava77 I am about to submit a PR with the last piece needed to run a whole production workflow with the new ETL (2 discs), which needs the updates. in this PR (as the thickness of the silicon is now realistically the 50 microns of the LGAD). When this enters in production, and geometry for HGCal provided by @bsunanda should go with it, so this modifier should become in my view an integral part of the baseline Era. I see the special version as a short term interim solution before moving to that new regime, as I expect that the review of the new code will take a bit of time

Copy link
Contributor

@kpedro88 kpedro88 Oct 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiocos @gsorrentino18 is the suggestion to go ahead with this PR without introducing a new modifier?
if so, there are still a few other unanswered comments; please address these

Copy link
Contributor Author

@gsorrentino18 gsorrentino18 Oct 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpedro88 My intention is to implement an Era Modifier, but since this PR is related to #31765 and #31769, which introduce bigger modifications in the code, I though it would be better to hold this PR until the review of the other two is completed.

Copy link
Contributor

@fabiocos fabiocos Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpedro88 @slava77 @bsunanda so what is the preferred strategy? Go for a short period with an intermediate Era and then move the ETL definition into a new Phase2CXX default? Or we may keep this PR on hold and use it for tests of #31765 with #31769, add an Era there and then merge everything in one shot. @silviodonato @qliphy please clarify the preferred strategy

Copy link
Contributor

@silviodonato silviodonato Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiocos I'm ok with having an intermediate Era

index_topology = 1; //2Disk geometry

/// position and positionError in unit cm
float position = -1.f;
float positionError = -1.f;
Expand Down Expand Up @@ -80,7 +94,7 @@ FTLRecHit MTDRecHitAlgo::makeRecHit(const FTLUncalibratedRecHit& uRecHit, uint32

// Now fill flags
// all rechits from the digitizer are "good" at present
if (energy > thresholdToKeep_) {
if (energy > thresholdToKeep_[index_topology]) {
flags = FTLRecHit::kGood;
rh.setFlag(flags);
} else {
Expand Down
4 changes: 2 additions & 2 deletions RecoLocalFastTime/FTLRecProducers/python/mtdRecHits_cfi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@

_barrelAlgo = cms.PSet(
algoName = cms.string("MTDRecHitAlgo"),
thresholdToKeep = cms.double(1.), # MeV
thresholdToKeep = cms.vdouble(1., 1.), # MeV
calibrationConstant = cms.double(0.03125), # MeV/pC
)


_endcapAlgo = cms.PSet(
algoName = cms.string("MTDRecHitAlgo"),
thresholdToKeep = cms.double(0.0425), # MeV
thresholdToKeep = cms.vdouble(0.0425, 0.005), # MeV
calibrationConstant = cms.double(0.085), # MeV/MIP
)

Expand Down