Format of RawTowerContainer id and RawTower id differ

[mccumbermike]

Perhaps someone can tell me which of these two objects is doing the right thing. RawTowerContainer considers the combined tower id to be (ieta << 16 | iphi) while the RawTower does (ieta << 12 | iphi). What is the expected behavior?

[blackcathj]

That is a fun discrepancy. As far as I can dig:

• RawTowerContainer::genkey(), which use (ieta << 16 | iphi) was imported from PHENIX.
• about one month ago, an id function was patched to RawTower(v1) through commit 235887adf7a35c0c743cfa8d84688845016b32d9, where a unsigned int get_id() const was introduced with bineta << 12 | binphi.

As long as I can see, it introduce no error in using them, since RawTowerContainer::keys are entirely internal to RawTowerContainer, and self-consistent.

Nevertheless, it is quite uncomfortable to have two formats of tower IDs. Therefore I would suggest the following solution:

• add a unsigned int id; to RawTowerv1, so it should be set via RawTowerContainer::AddTower().
• Change RawTowerv1::get_id() to read RawTowerv1::id.

Let me know if you think it is right way to go. I will be happy to patch it in if you want.

[mccumbermike]

My opinion is that we should pick a bit format and be done, either the 16 bit or 12 bit standard---this is obviously what was intended. Then the lookup key on the container will be the same as the object and we can fetch things quickly without a search and without a meaningful difference between iphi/ieta lookup and combined key lookup.

My understanding when adding the 12 bit standard to the object was this will be the standard used on the new towers. Can someone confirm that again?

Mike

Michael P. McCumber, PhD Los Alamos National Laboratory 505-709-8161

On Thu, Sep 3, 2015 at 5:16 PM, Jin Huang notifications@github.com wrote:

That is a fun discrepancy. As far as I can dig:

• RawTowerContainer::genkey(), which use (ieta << 16 | iphi) was imported from PHENIX.
• about one month ago, an id function was patched to RawTower(v1) through commit 235887a https://github.com/sPHENIX-Collaboration/coresoftware/commit/235887adf7a35c0c743cfa8d84688845016b32d9, where a unsigned int get_id() const was introduced with bineta << 12 | binphi.

As long as I can see, it introduce no error in using them, since RawTowerContainer::keys are entirely internal to RawTowerContainer, and self-consistent.

Nevertheless, it is quite uncomfortable to have two formats of tower IDs. Therefore I would suggest the following solution:

• add a unsigned int id; to RawTowerv1, so it should be set via RawTowerContainer::AddTower().
• Change RawTowerv1::get_id() to read RawTowerv1::id.

Let me know if you think it is right way to go. I will be happy to patch it in if you want.

— Reply to this email directly or view it on GitHub https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-137599013 .

[pinkenburg]

I am working on the side on my fork to get the towers and their id's under control. This was consistent some time ago but I assume this change must come from our Stony Brook idea to put some calo id into the upper 8 bits and use the lower 24 bits as unique id. For the barrel we key phi and eta into that, forward it's probably going to be something else. I assume the container wasn't updated accordingly and lookups in eta/phi will fail. It should affect the clustering - I wonder why nobody has noticed so far

On 9/3/2015 7:16 PM, Jin Huang wrote:

That is a fun discrepancy. As far as I can dig:

• RawTowerContainer::genkey(), which use (ieta << 16 | iphi) was imported from PHENIX.
• about one month ago, an id function was patched to RawTower(v1) through commit 235887a https://github.com/sPHENIX-Collaboration/coresoftware/commit/235887adf7a35c0c743cfa8d84688845016b32d9, where a unsigned int get_id() const was introduced with bineta << 12 | binphi.

As long as I can see, it introduce no error in using them, since RawTowerContainer::keys are entirely internal to RawTowerContainer, and self-consistent.

Nevertheless, it is quite uncomfortable to have two formats of tower IDs. Therefore I would suggest the following solution:

• add a unsigned int id; to RawTowerv1, so it should be set via RawTowerContainer::AddTower().
• Change RawTowerv1::get_id() to read RawTowerv1::id.

Let me know if you think it is right way to go. I will be happy to patch it in if you want.

— Reply to this email directly or view it on GitHub https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-137599013.

---

Christopher H. Pinkenburg ; pinkenburg@bnl.gov ; http://www.phenix.bnl.gov/~pinkenbu

Brookhaven National Laboratory ; phone: (631) 344-5692 Physics Department Bldg 510 C ; fax: (631) 344-3253 Upton, NY 11973-5000

---

[blackcathj]

@mccumbermike , I prefer 12bit, which leave room for future calo id tagging as mentioend by @pinkenburg . @pinkenburg , It appears to me that the 16bit key was internal to the RawTowerContainer only. Its interface use eta/phi bin strictly. Therefore, this discrepancy in conversion never shows up outside it, or affecting downstream code.

[mccumbermike]

The only feature I need is that the unique uint map key match the unique uint id of the tower to allow log(n) lookups and not cycling over the entire map looking for the unique id on the tower because it doesn't match the map key.

How the bits of the uint key are divided internally is unimportant to the evaluation I'm writing for the jets.

Michael P. McCumber, PhD Los Alamos National Laboratory 505-709-8161

On Thu, Sep 3, 2015 at 5:51 PM, Jin Huang notifications@github.com wrote:

@mccumbermike https://github.com/mccumbermike , I prefer 12bit, which leave room for future calo id tagging as mentioend by @pinkenburg https://github.com/pinkenburg . @pinkenburg https://github.com/pinkenburg , It appears to me that the 16bit key was internal to the RawTowerContainer only. Its interface use eta/phi bin strictly. Therefore, this discrepancy in conversion never shows up outside it, or affecting downstream code.

— Reply to this email directly or view it on GitHub https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-137603717 .

[blackcathj]

@mccumbermike , talked with @pinkenburg , here is a brief summary: Chris will first add the consistent lookup key for you to use in eval development very soon. Then we will rearrange the field a bit within RawTower to make it more compatible to the workflow of calorimeters (fiber/tiles -> sum visible energy -> convert to photo electron -> add noise -> store measured photo-electron or ADC -> later stage to calibrate to measured energy). It should work better with the develop branch from Nils too (probably merging after pre-CDR). Nevertheless, to users of RawTower , the main change probably just only the name of the RawTower::get_edep() function.

[mccumbermike]

Excellent. I'm just about ready to start runtime debugging the jet evaluator, so that will be very timely.

Michael P. McCumber, PhD Los Alamos National Laboratory 505-709-8161

On Fri, Sep 4, 2015 at 9:38 AM, Jin Huang notifications@github.com wrote:

@mccumbermike https://github.com/mccumbermike , talked with @pinkenburg https://github.com/pinkenburg , here is a brief summary: Chris will first add the consistent lookup key for you to use in eval development very soon. Then we will rearrange the field a bit within RawTower to make it more compatible to the workflow of calorimeters (fiber/tiles -> sum visible energy -> convert to photo electron -> add noise -> store measured photo-electron or ADC -> later stage to calibrate to measured energy). It should work better with the develop branch from Nils too (probably merging after pre-CDR). Nevertheless, to users of RawTower , the main change probably just only the name of the RawTower::get_edep() function.

— Reply to this email directly or view it on GitHub https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-137770967 .

[pinkenburg]

it should be done now. But when running this I noticed that there is only a very limited phi range for the inner hcal. I assume something goes wrong in the cell reconstruction (old and new towering behaved identical) that only slat #0 is summed up. As it stands there won't be towers from the inner hcal until this is fixed. I am not sure if I get around looking at this during this weekend. Otherwise it's the first thing for me next week.

On 9/4/2015 11:47 AM, Michael P. McCumber wrote:

Excellent. I'm just about ready to start runtime debugging the jet evaluator, so that will be very timely.

Michael P. McCumber, PhD Los Alamos National Laboratory 505-709-8161

On Fri, Sep 4, 2015 at 9:38 AM, Jin Huang notifications@github.com wrote:

@mccumbermike https://github.com/mccumbermike , talked with @pinkenburg https://github.com/pinkenburg , here is a brief summary: Chris will first add the consistent lookup key for you to use in eval development very soon. Then we will rearrange the field a bit within RawTower to make it more compatible to the workflow of calorimeters (fiber/tiles -> sum visible energy -> convert to photo electron -> add noise -> store measured photo-electron or ADC -> later stage to calibrate to measured energy). It should work better with the develop branch from Nils too (probably merging after pre-CDR). Nevertheless, to users of RawTower , the main change probably just only the name of the RawTower::get_edep() function.

— Reply to this email directly or view it on GitHub

https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-137770967 .

— Reply to this email directly or view it on GitHub https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-137772999.

---

Christopher H. Pinkenburg ; pinkenburg@bnl.gov ; http://www.phenix.bnl.gov/~pinkenbu

Brookhaven National Laboratory ; phone: (631) 344-5692 Physics Department Bldg 510 C ; fax: (631) 344-3253 Upton, NY 11973-5000

---

[mccumbermike]

Okay, so the implementation of RawTowerDefs::tower_idbits introduced a 3rd standard. :) It resolves to 24bits, so (ieta << 24 | iphi). This must be a bug. Also, RawTowerv1.h and some other places were still hard coded to 12.

So I fixed this by coding up an eta_idbits that is tower_idbits/2 and finished implementing the 12-bit standard in these other places---I think I've gotten them all in g4cemc. I'll put these patches into a pull request in a few minutes.

[pinkenburg]

the tower_idbits are intended to be the bits available for the encoded tower id (32-minus 8 bits for the calorimeter id), not the bit shift for eta. How those bits are used to create a unique id is up to the implementation. For the barrel it's eta/phi for the forward calorimeters it'll be something else. That doesn't mean that there shouldn't be a define for eta/phi shifts which is a good improvement.

On 9/5/2015 4:32 PM, Michael P. McCumber wrote:

Okay, so the implementation of RawTowerDefs::tower_idbits introduced a 3rd standard. :) It resolves to 24bits, so (ieta << 24 | iphi). This must be a bug. Also, RawTowerv1.h and some other places were still hard coded to 12.

So I fixed this by coding up an eta_idbits that is tower_idbits/2 and finished implementing the 12-bit standard in these other places---I think I've gotten them all in g4cemc. I'll put these patches into a pull request in a few minutes.

— Reply to this email directly or view it on GitHub https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-137993164.

---

Christopher H. Pinkenburg ; pinkenburg@bnl.gov ; http://www.phenix.bnl.gov/~pinkenbu

Brookhaven National Laboratory ; phone: (631) 344-5692 Physics Department Bldg 510 C ; fax: (631) 344-3253 Upton, NY 11973-5000

---

[mccumbermike]

Well then I think I no longer understand what the plan is for supporting the Add & getTower(ieta,iphi) interface on the container if the contained towers will define their key formats differently...

Also the bug on L22 of the RawTowerContainer.cc remains where the tower_idbits were used to shift the eta portion of the key which was causing the eval to crash.

I'll withdraw the other pull request where I had also solved these problems.

Michael P. McCumber, PhD Los Alamos National Laboratory 505-709-8161

On Sat, Sep 5, 2015 at 3:35 PM, pinkenburg notifications@github.com wrote:

the tower_idbits are intended to be the bits available for the encoded tower id (32-minus 8 bits for the calorimeter id), not the bit shift for eta. How those bits are used to create a unique id is up to the implementation. For the barrel it's eta/phi for the forward calorimeters it'll be something else. That doesn't mean that there shouldn't be a define for eta/phi shifts which is a good improvement.

On 9/5/2015 4:32 PM, Michael P. McCumber wrote:

Okay, so the implementation of RawTowerDefs::tower_idbits introduced a 3rd standard. :) It resolves to 24bits, so (ieta << 24 | iphi). This must be a bug. Also, RawTowerv1.h and some other places were still hard coded to 12.

So I fixed this by coding up an eta_idbits that is tower_idbits/2 and finished implementing the 12-bit standard in these other places---I think I've gotten them all in g4cemc. I'll put these patches into a pull request in a few minutes.

— Reply to this email directly or view it on GitHub < https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-137993164 .

---

Christopher H. Pinkenburg ; pinkenburg@bnl.gov ; http://www.phenix.bnl.gov/~pinkenbu

Brookhaven National Laboratory ; phone: (631) 344-5692 Physics Department Bldg 510 C ; fax: (631) 344-3253 Upton, NY 11973-5000

---

— Reply to this email directly or view it on GitHub https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-137997313 .

[pinkenburg]

L22 of the RawTowerContainer is a real bug - it should have been 12 (or a new def). I am sure there are more than this but the current code is not sensitive to indexing problems as long as they are unique so it's hard to test. I'll patch up this place and add this eta_bin = towerbins/2. Having methods to deal with eta and phi binning is very convenient for the barrel calorimeters and I see no reason to get rid of them (if this can be supported).

On 9/5/2015 5:57 PM, Michael P. McCumber wrote:

Well then I think I no longer understand what the plan is for supporting the Add & getTower(ieta,iphi) interface on the container if the contained jets will define their key formats differently...

Also the bug on L22 of the RawTowerContainer.cc remains where the tower_idbits were used to shift the eta portion of the key which was causing the eval to crash.

I'll withdraw the other pull request where I had also solved these problems.

Michael P. McCumber, PhD Los Alamos National Laboratory 505-709-8161

On Sat, Sep 5, 2015 at 3:35 PM, pinkenburg notifications@github.com wrote:

the tower_idbits are intended to be the bits available for the encoded tower id (32-minus 8 bits for the calorimeter id), not the bit shift for eta. How those bits are used to create a unique id is up to the implementation. For the barrel it's eta/phi for the forward calorimeters it'll be something else. That doesn't mean that there shouldn't be a define for eta/phi shifts which is a good improvement.

On 9/5/2015 4:32 PM, Michael P. McCumber wrote:

Okay, so the implementation of RawTowerDefs::tower_idbits introduced a 3rd standard. :) It resolves to 24bits, so (ieta << 24 | iphi). This must be a bug. Also, RawTowerv1.h and some other places were still hard coded to 12.

So I fixed this by coding up an eta_idbits that is tower_idbits/2 and finished implementing the 12-bit standard in these other places---I think I've gotten them all in g4cemc. I'll put these patches into a pull request in a few minutes.

— Reply to this email directly or view it on GitHub <

https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-137993164

.

---

Christopher H. Pinkenburg ; pinkenburg@bnl.gov ; http://www.phenix.bnl.gov/~pinkenbu

Brookhaven National Laboratory ; phone: (631) 344-5692 Physics Department Bldg 510 C ; fax: (631) 344-3253 Upton, NY 11973-5000

---

— Reply to this email directly or view it on GitHub

https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-137997313 .

— Reply to this email directly or view it on GitHub https://github.com/sPHENIX-Collaboration/coresoftware/issues/31#issuecomment-138002636.

---

Christopher H. Pinkenburg ; pinkenburg@bnl.gov ; http://www.phenix.bnl.gov/~pinkenbu

Brookhaven National Laboratory ; phone: (631) 344-5692 Physics Department Bldg 510 C ; fax: (631) 344-3253 Upton, NY 11973-5000

---

[mccumbermike]

Okay, now we are self-consistent. Thanks.