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

Replace CalorimeterScFiDigi with CalorimeterHitDigi, implement merging at digi step #491

Merged
merged 4 commits into from
Feb 16, 2023

Conversation

Chao1009
Copy link
Contributor

Briefly, what does this PR introduce?

It replaces the algorithm CalorimeterScFiDigi with CaloriemterHitDigi because the former breaks the production run of arches configuration.

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

It should not affect the default use of CalorimeterHitDigi. However, it implements the merging at the digitization step, which can be enabled by using the required parameters.

Does this PR change default behavior?

It should not affect the default use of CalorimeterHitDigi.

@Chao1009 Chao1009 changed the title Replace CalorimeterScFiDigi with CalorimeterHitDigi, implement merging at digi step Draft: Replace CalorimeterScFiDigi with CalorimeterHitDigi, implement merging at digi step Feb 16, 2023
@Chao1009
Copy link
Contributor Author

Chao1009 commented Feb 16, 2023

It's still a draft, testing on my local copy now.

@wdconinc wdconinc requested a review from veprbl February 16, 2023 16:31
@wdconinc
Copy link
Contributor

Added @veprbl for review, knowing this is still a draft.

@Chao1009 Chao1009 changed the title Draft: Replace CalorimeterScFiDigi with CalorimeterHitDigi, implement merging at digi step Replace CalorimeterScFiDigi with CalorimeterHitDigi, implement merging at digi step Feb 16, 2023
@Chao1009
Copy link
Contributor Author

Now it only throws a warning and switches to single-hit digi mode, if a provided field does not exist.

IDDescriptor     ERROR dd4hep: EcalBarrelScFiHits: This ID descriptor has no field with the name: something_off
[2023-02-16 11:34:35.372] [EcalBarrelScFiRawHits] [warning] Failed to load ID decoder for EcalBarrelScFiHits, hits will not be merged.

@Chao1009 Chao1009 force-pushed the implement_digi_merge branch from 4bfe8f4 to 7c6179e Compare February 16, 2023 17:41
@Chao1009
Copy link
Contributor Author

It 's ready

@wdconinc
Copy link
Contributor

File size certainly looks a lot better.
image

Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

Looks good to me. Waiting for @veprbl's approval.

@Chao1009 Chao1009 force-pushed the implement_digi_merge branch from 5add1ea to 7c6179e Compare February 16, 2023 21:16
@veprbl
Copy link
Member

veprbl commented Feb 16, 2023

Please merge, if this is ready.

@wdconinc wdconinc enabled auto-merge February 16, 2023 22:31
@wdconinc wdconinc merged commit 49bbf11 into main Feb 16, 2023
@wdconinc wdconinc deleted the implement_digi_merge branch February 16, 2023 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants