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

fix: propagate parameters from reco_flags to cpp plugins #408

Merged
merged 16 commits into from
Jan 14, 2023

Conversation

wdconinc
Copy link
Contributor

Briefly, what does this PR introduce?

This addresses a lot of the discrepancies in https://eic.github.io/EICrecon/#/table_flags/flags_view

What kind of change does this PR introduce?

  • Bug fix (issue: reco_flags duplicates parameters)
  • New feature (issue #__)
  • Documentation update
  • Other: __

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?

No.

Does this PR change default behavior?

No.

@wdconinc
Copy link
Contributor Author

Use the pion flags file for diffs to have higher probability that Hcal factories are initialized.

DraTeots
DraTeots previously approved these changes Jan 6, 2023
@c-dilks
Copy link
Member

c-dilks commented Jan 6, 2023

There are still some differences at https://wdconinc.github.io/EICrecon/#/table_flags/flags_view, is this the latest version? Also, I will take care of the DRICH ones in a separate PR.

@wdconinc
Copy link
Contributor Author

wdconinc commented Jan 6, 2023

I think my main is pointing to a different commit by now.

@wdconinc
Copy link
Contributor Author

wdconinc commented Jan 6, 2023

Ah, no, my main is pointing to a but the last commit here. The remaining differences in protoclusters is still under investigation. But I think this is still ready for merging.

@wdconinc
Copy link
Contributor Author

wdconinc commented Jan 6, 2023

There is also the issue that the gh-pages can only show either arches or brycecanyon, and EICrecon only initializes a factory when needed. So part of the chain is just not exercised.

@c-dilks
Copy link
Member

c-dilks commented Jan 7, 2023

I believe these are the remaining differences:

#
#key                                                           => reco_flags.py as is   VS reco_flags.py with no overrides (empty `eicrecon_reco_flags` list)
#
BEMC:EcalBarrelImagingRawHits:readoutClass                     => pixel                 VS
BEMC:EcalBarrelScFiRawHits:energyResolutions                   =>                       VS 0
BEMC:EcalBarrelScFiRawHits:readoutClass                        => light_guide           VS
EEMC:EcalEndcapPInsertIslandProtoClusters:dimScaledLocalDistXY => 1.5,1.5               VS 1.8,1.8
EEMC:EcalEndcapPInsertIslandProtoClusters:localDistXY          => 10,10                 VS
EEMC:EcalEndcapPInsertIslandProtoClusters:minClusterCenterEdep => 0.01                  VS 0.03
EEMC:EcalEndcapPInsertIslandProtoClusters:minClusterHitEdep    => 0                     VS 0.001
EEMC:EcalEndcapPInsertRecHits:capacityADC                      => 16384                 VS 8096
EEMC:EcalEndcapPInsertRecHits:dynamicRangeADC                  => 3.0                   VS 0.1
EEMC:EcalEndcapPInsertRecHits:pedestalMean                     => 100                   VS 400
EEMC:EcalEndcapPInsertRecHits:pedestalSigma                    => 0.7                   VS 3.2
EEMC:EcalEndcapPInsertRecHits:samplingFraction                 => 0.03                  VS 0.998
EEMC:EcalEndcapPInsertRecHits:thresholdFactor                  => 5.0                   VS 4
EEMC:EcalEndcapPInsertRecHits:thresholdValue                   => 2                     VS 0
EEMC:EcalEndcapPInsertTruthClusters:enableEtaBounds            => 1                     VS 0
EEMC:EcalEndcapPInsertTruthClusters:logWeightBase              => 6.2                   VS 3.6
EEMC:EcalEndcapPIslandProtoClusters:dimScaledLocalDistXY       => 1.5,1.5               VS 1.8,1.8
EEMC:EcalEndcapPIslandProtoClusters:localDistXY                => 10,10                 VS
EEMC:EcalEndcapPIslandProtoClusters:minClusterCenterEdep       => 0.01                  VS 0.03
EEMC:EcalEndcapPIslandProtoClusters:minClusterHitEdep          => 0                     VS 0.001
HCAL:HcalBarrelIslandProtoClusters:localDistXY                 => 150,150               VS 1.5,1.5
HCAL:HcalEndcapNIslandProtoClusters:localDistXY                => 150,150               VS 1.5,1.5
HCAL:HcalEndcapPInsertIslandProtoClusters:localDistXY          =>                       VS 1.5,1.5
HCAL:HcalEndcapPInsertRecHits:samplingFraction                 => 0.998                 VS 1
HCAL:HcalEndcapPIslandProtoClusters:localDistXY                => 150,150               VS 1.5,1.5
tracking:CentralCKFTrajectories:Chi2CutOff                     => 15                    VS 50

New issue+PR or fix them here?

c-dilks
c-dilks previously approved these changes Jan 7, 2023
@wdconinc
Copy link
Contributor Author

HCAL:HcalBarrelIslandProtoClusters:localDistXY                 => 150,150               VS 1.5,1.5
HCAL:HcalEndcapNIslandProtoClusters:localDistXY                => 150,150               VS 1.5,1.5
HCAL:HcalEndcapPIslandProtoClusters:localDistXY                => 150,150               VS 1.5,1.5

That just can't be right. Do we really want localDistXY to be 150 cm?

@wdconinc wdconinc dismissed stale reviews from c-dilks and DraTeots via 56561af January 12, 2023 03:26
@wdconinc
Copy link
Contributor Author

wdconinc commented Jan 12, 2023

Remaining (along with the HCAL localDistXY above) are

BEMC:EcalBarrelImagingRawHits:readoutClass                     => pixel                 VS
BEMC:EcalBarrelScFiRawHits:energyResolutions                   =>                       VS 0
BEMC:EcalBarrelScFiRawHits:readoutClass                        => light_guide           VS

@wdconinc wdconinc requested a review from c-dilks January 12, 2023 04:13
@c-dilks
Copy link
Member

c-dilks commented Jan 12, 2023

Now we are down to:

BEMC:EcalBarrelImagingRawHits:readoutClass            => pixel                 VS
BEMC:EcalBarrelScFiRawHits:energyResolutions          =>                       VS 0
BEMC:EcalBarrelScFiRawHits:readoutClass               => light_guide           VS

EEMC:EcalEndcapPInsertIslandProtoClusters:localDistXY => 10,10                 VS 1,1
EEMC:EcalEndcapPIslandProtoClusters:localDistXY       => 10,10                 VS 1,1

HCAL:HcalBarrelIslandProtoClusters:localDistXY        => 150,150               VS 1.5,1.5
HCAL:HcalEndcapNIslandProtoClusters:localDistXY       => 150,150               VS 1.5,1.5
HCAL:HcalEndcapPInsertIslandProtoClusters:localDistXY =>                       VS 1.5,1.5
HCAL:HcalEndcapPIslandProtoClusters:localDistXY       => 150,150               VS 1.5,1.5

I suppose EEMC and HCAL differences are caused by #433.

@wdconinc
Copy link
Contributor Author

wdconinc commented Jan 13, 2023

Now we are down to:

 BEMC:EcalBarrelImagingRawHits:readoutClass            => pixel                 VS
BEMC:EcalBarrelScFiRawHits:readoutClass               => light_guide           VS

Afaik this value is unused, and pixel or light_guide are not even classes. As introduced by e6f1f93, @faustus123, does this do something new in EICrecon?

BEMC:EcalBarrelScFiRawHits:energyResolutions          =>                       VS 0

I'd argue this is not right. Every other calorimeter has a non-zero energy resolution term, so this may need to be {0,0.02,0} to allow fair comparison with the other barrel calorimeter. @mariakzurek?

EEMC:EcalEndcapPInsertIslandProtoClusters:localDistXY => 10,10                 VS 1,1
EEMC:EcalEndcapPIslandProtoClusters:localDistXY       => 10,10                 VS 1,1

✓ I'll fix those. I keep thinking in mm, not cm, as the basic unit.

HCAL:HcalBarrelIslandProtoClusters:localDistXY        => 150,150               VS 1.5,1.5
HCAL:HcalEndcapNIslandProtoClusters:localDistXY       => 150,150               VS 1.5,1.5
HCAL:HcalEndcapPInsertIslandProtoClusters:localDistXY =>                       VS 1.5,1.5
HCAL:HcalEndcapPIslandProtoClusters:localDistXY       => 150,150               VS 1.5,1.5

I suppose EEMC and HCAL differences are caused by #433.

There is even more wrong here than meets the eye. The intention is that only one of the localDistXY etc pairs is supposed to be set. If multiple are set, then the last one in the arbitrary list in the algorithm is used, in this case dimScaledLocalDistXY, and that one should not have dimensions. So, I'm going to punt on fixing Hcal clustering for this PR.

@wdconinc
Copy link
Contributor Author

@faustus123
Copy link
Contributor

Afaik this value is unused, and pixel or light_guide are not even classes. As introduced by e6f1f93, @faustus123, does this do something new in EICrecon?

I cannot recall where those came from. I just poked around in my old work directories, but am not seeing "pixel" show up in this context. It is a mystery to me. So is this something that should be left as an empty string?

@wdconinc
Copy link
Contributor Author

So is this something that should be left as an empty string?

Hits refer to CellIDs. The CellID includes bits that define the detector. The detector can have multiple readout collections, but typically has only one. When there are multiple readout collections for a detector, then the readoutClass field is used to choose which readout collection to use. That does not apply here, and I think an empty string should be used.

@faustus123
Copy link
Contributor

OK. Since they are already empty strings in the library code, then I take it no action is needed. You just wanted verification that there was not a reason why they should actually be set to "pixel" and "light_guide". Let me know if this is incorrect and you need more from me.

@mariakzurek
Copy link
Contributor

I'd argue this is not right. Every other calorimeter has a non-zero energy resolution term, so this may need to be {0,0.02,0} to allow fair comparison with the other barrel calorimeter. @mariakzurek?

This is right only for the Imaging Calorimetr; I guess it has been copied for other calorimeters from the imaging layers. It's 2% flat resolution on single pixel readout for AstroPix. @Chao1009 @sly2j

@wdconinc
Copy link
Contributor Author

Verified with @mariakzurek that BEMC:EcalBarrelScFiRawHits:energyResolutions should be 0 as in the cpp file right now, not empty string '' as in the reco_flags (since that is not actually a specification of a value, just a deferral to the default in the cpp file).

@wdconinc
Copy link
Contributor Author

So, only remaining issue is:

HCAL:HcalBarrelIslandProtoClusters:localDistXY        => 150,150               VS 1.5,1.5
HCAL:HcalEndcapNIslandProtoClusters:localDistXY       => 150,150               VS 1.5,1.5
HCAL:HcalEndcapPInsertIslandProtoClusters:localDistXY =>                       VS 1.5,1.5
HCAL:HcalEndcapPIslandProtoClusters:localDistXY       => 150,150               VS 1.5,1.5

@wdconinc wdconinc force-pushed the remove-reco-flags-diffs branch from 544c1c3 to b88c8b6 Compare January 14, 2023 16:28
@wdconinc wdconinc requested a review from DraTeots January 14, 2023 16:39
@wdconinc wdconinc merged commit f296eba into main Jan 14, 2023
@wdconinc wdconinc deleted the remove-reco-flags-diffs branch January 14, 2023 23:44
@c-dilks c-dilks mentioned this pull request Jan 18, 2023
7 tasks
veprbl added a commit that referenced this pull request Apr 4, 2023
The MeV multipliers were introduced in #408, but they don't make sense from the dimensional analysis point (terms are divided by sqrt(E), 1 and E respectively). Each element of the array would need to have its own unit, but the commonly accepted convention is to use fractions and assume GeV. This commit restores that convention. The actual behavior in reconstruction is not changed by this PR, because the values stay the same.
veprbl added a commit that referenced this pull request Apr 5, 2023
The MeV multipliers were introduced in #408, but they don't make sense from the dimensional analysis point (terms are divided by sqrt(E), 1 and E respectively). Each element of the array would need to have its own unit, but the commonly accepted convention is to use fractions and assume GeV. This commit restores that convention. The actual behavior in reconstruction is not changed by this PR, because the values stay the same.
veprbl added a commit that referenced this pull request Apr 5, 2023
The MeV multipliers were introduced in #408, but they don't make sense
from the dimensional analysis point (terms are divided by sqrt(E), 1 and
E respectively). Each element of the array would need to have its own
unit, but the commonly accepted convention is to use fractions and
assume GeV. This commit restores that convention. The actual behavior in
reconstruction is not changed by this PR, because the values stay the
same.
veprbl added a commit that referenced this pull request Apr 6, 2023
The MeV multipliers were introduced in #408, but they don't make sense
from the dimensional analysis point (terms are divided by sqrt(E), 1 and
E respectively). Each element of the array would need to have its own
unit, but the commonly accepted convention is to use fractions and
assume GeV. This commit restores that convention. The actual behavior in
reconstruction is not changed by this PR, because the values stay the
same.
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.

5 participants