Skip to content
This repository has been archived by the owner on May 9, 2024. It is now read-only.

Bringing back the old digi/rec-producer for testing/debugging purposes #75

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

EinarElen
Copy link
Contributor

@EinarElen EinarElen commented Apr 7, 2024

This resolves LDMX-Software/ldmx-sw#1351 by adding a new producer called HcalSimpleDigiAndRecProducer. It takes simulation level information and performs rudimentary reconstruction, i.e: for each bar, make a hit where energy/PE, position, and time are a weighted average of the corresponding simhits (weighted by energy deposited).

This verision does not handle noise hits at the moment, but we could introduce that.

I think this is ready for being reviewed, although it needs some polishing (e.g. documentation/comments) before being merged. It could already be useful for some people so I'm creating the PR already.

Below is the hcal DQM plots comparing the regular hcal reco chain with the naive/simple one.

figures.zip

  • PE distributions do not feature ADC saturation

hcal_dqm_back_pe

hcal_dqm_back_max_pe

hcal_dqm_back_total_pe

  • Position along the bars is obviously not well handled by this approach, several hits are far outside of the actual Hcal :)
    hcal_dqm_back_along_x

  • Hit times are not limited to the 8 time samples, so should also be taken with a bucket of salt
    hcal_dqm_back_hit_time

  • Hit multiplicity is different, which i suspect is from reading out some hits that are out of time or just that the readout threshold is lower. The important part (vetoable hits) is more similar
    hcal_dqm_back_hit_multiplicity
    hcal_dqm_back_vetoable_hit_multiplicity

  • Lastly, I'm a bit curious about the inefficiency figures. In general, the simple reconstruction tends to give us better inefficiencies which probably is optimistic
    HcalInefficiencyAnalyzer_efficiency
    HcalInefficiencyAnalyzer_inefficiency_bottom
    HcalInefficiencyAnalyzer_inefficiency_back

Copy link
Collaborator

@cmantill cmantill left a comment

Choose a reason for hiding this comment

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

hi @EinarElen - this looks like a good idea and the validation plots look good to me.

Just for curiosity, have you played with the readout or in-time thresholds? Curious to see whether the distribution number of hits looks more similar if these are varied. I agree that the important distribution of vetoable hits looks more similar so perhaps that is better, but perhaps it might still be better to get rid of the small energy hits for clustering or visible search purposes.

@EinarElen
Copy link
Contributor Author

hi @EinarElen - this looks like a good idea and the validation plots look good to me.

Just for curiosity, have you played with the readout or in-time thresholds? Curious to see whether the distribution number of hits looks more similar if these are varied. I agree that the important distribution of vetoable hits looks more similar so perhaps that is better, but perhaps it might still be better to get rid of the small energy hits for clustering or visible search purposes.

Not really, and on the current version there isn't really a in-time threshold at all (I can go ahead and add something like that though) :)

Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

I don't really have any comments on the algorithm. I think it makes sense and your plots convince me that there aren't glaring bugs lurking.

I do have a minor suggestion on the technical side. We use C++17 std, so we can take advantage of emplace_back's reference return allowing us to avoid an extra move construction required by push_back. Maybe not a super big performance boost, but I like the look of writing code this way (each barID gets a new hit which we then edit with the specifics in that bar).

src/Hcal/HcalSimpleDigiAndRecProducer.cxx Outdated Show resolved Hide resolved
src/Hcal/HcalSimpleDigiAndRecProducer.cxx Outdated Show resolved Hide resolved
@tomeichlersmith
Copy link
Member

With the impending submodule->subdirectory transition on Wednesday, I am commenting here to remind you to either merge this PR before then or close it and re-apply the changes after Wednesday to make a new PR on the subdirectory of ldmx-sw.

@cmantill
Copy link
Collaborator

cmantill commented May 1, 2024

@EinarElen , do you want to answer to Tom's comments? Or should we leave this PR for the subdirectory?

@EinarElen
Copy link
Contributor Author

I can send a patch tonight

@tomeichlersmith
Copy link
Member

I have already done the transition and am in the final stages of testing. Updating submodule changes to apply to subdirectories is not a difficult process and one that I've tested on my own branch of the ECal submodule. Please follow those instructions and re-open this PR in ldmx-sw off the new trunk.

@EinarElen
Copy link
Contributor Author

EinarElen commented May 1, 2024 via email

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

Successfully merging this pull request may close these issues.

Bring back the old simple reconstruction chain
3 participants