search

gemc / source

gemc website:
gemc.jlab.org
14 stars 73 forks source link

Remove unnecessary check #116

Closed agoose77 closed 7 years ago

agoose77 commented 7 years ago

Should this condition every be true? The mirror handling is done in sensitivity not hitType, so this should never happen.

https://github.com/gemc/source/blob/6d247fb113eb27647e7c8f0063339d59277d4941/src/MEventAction.cc#L420

maureeungaro commented 7 years ago

The sensitivity can be "mirror:" (assigned in the geometry). In this case the hitType in event action is reverted to mirror so it can pick it in the hit process map.

This mechanism is indeed a bit clunky. It was added to allow recording hit positions for photons bouncing off mirrors.

In gemc 3 the syntax is much less confusing and there are no ambiguities.

agoose77 commented 7 years ago

Isn't this the hittype though? I'm aware of the mirror mechanism (I'm using it), but IIRC this is not the sensitivity variable.

maureeungaro commented 7 years ago

Yes, it is hittype. I should have said:

The hittype can be "mirror:" (assigned in the geometry). In this case the hitType in event action is reverted to mirror so it can pick it in the hit process map.

In event action the variable "hitType" is used to pick the right digitization routine from the factory map. Alternatively I could add a line in the register:

    hitMap["mirror"]         = &flux_HitProcess::createHitClass;
    hitMap["mirror:"]         = &flux_HitProcess::createHitClass;

Which would essentially do the same thing. As I said, this mechanism is far from ideal (was a quick patch) but it is addressed in gemc 3.

agoose77 commented 7 years ago

Sorry, what I'm saying is that the variable "hitType" should never be "mirror:". It can be "mirror", but the "mirror: mirror_name" syntax is used only for sensitivity.

So, to recap: We search for "mirror:" in sensitivity

  1. https://github.com/gemc/source/blob/master/src/MDetectorConstruction.cc#L245
  2. https://github.com/gemc/source/blob/master/src/MDetectorConstruction.cc#L456

And 1. changes the sensitivity to "flux"

But, then https://github.com/gemc/source/blob/master/sensitivity/HitProcess.cc#L13 checks for "mirror:" in the hitType variable. Why does it do this? We should get the mirror name from sensitivity, not hitType?

The same thing here: https://github.com/gemc/source/blob/master/src/MEventAction.cc#L420, we're checking hitType, not sensitivity?

Apologies if i've missed something

maureeungaro commented 7 years ago

Good catch.

I misunderstood the issue. The checks in HitProcess and EventAction are on hitType. That is correct since that's the name (key) in the map<string, HitProcess_Factory> hitMap.

However you are correct:

In both HitProcess and EventAction there is no need of this check. The routine is registered under "mirror", and hitType will never be "mirror:".

maureeungaro commented 7 years ago

Fixed in the repository.

agoose77 commented 7 years ago

I have since determined why this check was in place:

cad_det_factory.cc:201 and gdml_det_factory.cc:148 both revert the hitType to the sensitivity if it is not given.

best revert this for now @maureeungaro

maureeungaro commented 7 years ago

Agree. Modified in the repository: hitType is not copied anymore, its value must be explicitly requested by user.