Skip to content
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

Correct some inconsistencies in V16 geometry #117

Open
wants to merge 2 commits into
base: hgc-tpg-devel-CMSSW_14_0_0_pre1
Choose a base branch
from

Conversation

EmyrClement
Copy link

PR description:

This PR fixes two inconsistencies with the V16 trigger geometry:

  • The wafer centring of the CE-H were assumed to all be corner centred, but some are not wafer centred. These cases are checked against the information from the topology class.
  • HGCalTriggerModuleDetId only distinguishes between fine and coarse scintillator modules, however HGCScintillatorDetId also distinguishes between type "c" and "m" of coarse modules. For now, I removed this distinction.

PR validation:

Checked the number of errorbits reported by the HGCalTriggerGeoTesterBackendStage1 tester has reduced. There are still inconsistencies, but fewer than before.

Copy link

@jbsauvan jbsauvan left a comment

Choose a reason for hiding this comment

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

I'm wondering, since there is potentially some other changes between $<V16$ and $\geq V16$ geometries (potentially incompatible changes?), if it would make sense to write a new geometry class, to be used for post-V16 geometries ([...]GeometryV16[...])?

Comment on lines +972 to +984
// Check topology for wafer centred layers, otherwise CornerCentredY
if (topologyLayerType == HGCalTypes::WaferCenter || topologyLayerType == HGCalTypes::WaferCenterR ||
topologyLayerType == HGCalTypes::WaferCenterB)
return HGCalGeomRotation::WaferCentring::WaferCentred;
else
return HGCalGeomRotation::WaferCentring::CornerCentredY;
} else { // CE-H Even
return HGCalGeomRotation::WaferCentring::CornerCentredMercedes;
// Check topology for wafer centred layers, otherwise CornerCentredMercedes
if (topologyLayerType == HGCalTypes::WaferCenter || topologyLayerType == HGCalTypes::WaferCenterR ||
topologyLayerType == HGCalTypes::WaferCenterB)
return HGCalGeomRotation::WaferCentring::WaferCentred;
else
return HGCalGeomRotation::WaferCentring::CornerCentredMercedes;

Choose a reason for hiding this comment

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

Do we still need to separate between odd and even layers if we can retrieve the type of layer centring with dddConstants.layerType()?

Copy link
Author

Choose a reason for hiding this comment

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

Looking at where dddConstants.layerType() retrieves the layer type:

int32_t HGCalTypes::layerType(int type) {
static constexpr int32_t types[5] = {HGCalTypes::WaferCenter,
HGCalTypes::WaferCenterB,
HGCalTypes::CornerCenterYp,
HGCalTypes::CornerCenterYm,
HGCalTypes::WaferCenterR};
return ((type >= 0 && type < 5) ? types[type] : HGCalTypes::WaferCenter);

If HGCalTypes::CornerCenterYp and HGCalTypes::CornerCenterYm uniquely correspond to one of HGCalGeomRotation::WaferCentring::CornerCentredY and HGCalGeomRotation::WaferCentring::CornerCentredMercedes, then I think we can remove the separation between odd and even layers. Though I don't know which of CornerCenterYp/m corresponds to CornerCentredY/Mercedes - I can work this out, unless you happen to know.

Choose a reason for hiding this comment

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

I would naively guess that Yp is Y and Ym is Mercedes (negative version of Y), but not sure at all.

@EmyrClement
Copy link
Author

@jbsauvan I see that the jenkins tests have failed when running the cmssw config with these changes. However I can't access the link to get more information to understand why/see a more detailed log - are you able to see what happened?

@jbsauvan
Copy link

Hi @EmyrClement
I checked on Jenkins but cannot find more info. Seems like something related to the validation code and unrelated to this PR. I'm checking with the validation developer.

@jbsauvan I see that the jenkins tests have failed when running the cmssw config with these changes. However I can't access the link to get more information to understand why/see a more detailed log - are you able to see what happened?

@EmyrClement
Copy link
Author

Thanks - if it's of any use, I see the same error with #113

Hi @EmyrClement I checked on Jenkins but cannot find more info. Seems like something related to the validation code and unrelated to this PR. I'm checking with the validation developer.

@jbsauvan I see that the jenkins tests have failed when running the cmssw config with these changes. However I can't access the link to get more information to understand why/see a more detailed log - are you able to see what happened?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants