Add a steering file for monitoring using KF tracks and add a tool to remove hits from pattern reco in LCIO #935
Conversation
| SiSensor sensor = (SiSensor) rawHit.getDetectorElement(); | ||
| String name = sensor.getName(); | ||
|
|
||
| if (sensorNames_.size() > 0) { |
There was a problem hiding this comment.
This check is unnecessary. If sensorNames_ is empty, the whole for loop will be skipped.
There was a problem hiding this comment.
The loop is over all the hits, this check is on the list of sensors we want to skip. I do think sensorNames_ is potentially a confusing variable name since it is really the sensors to be skipped.
| String name = sensor.getName(); | ||
|
|
||
| if (sensorNames_.size() > 0) { | ||
|
|
There was a problem hiding this comment.
I think the following works as well ...
for (LCRelation fittedHit : fittedHits) {
RawTrackerHit rawHit = FittedRawTrackerHit.getRawTrackerHit(fittedHit);
SiSensor sensor = (SiSensor) rawHit.getDetectorElement();
if (sensorNames_.contains(sensor.name()) continue;
sensor.getReadout().addHit(fittedHit);
}
Note the use of the contains method. When using a HashSet, this gives you O(1) lookup time vs O(n) when using an ArrayList or just iterating over the elements. HashSet is appropriate here since all of the sensor names will be unique. It also might be worth changing the name of the set sensorNames_ to maskedSensorNames_ so it's clearer what it does.
There was a problem hiding this comment.
The behavior of this will be different. contains behavior is different when called on an array vs a string. I think we do indeed want contains called on the string and not the array, which gives us easier control over which sensors we drop. This is only called once, so the complexity of the code doesn't really matter, and either way will effectively run in the same time. We would like to be able to remove multiple sensors with a single string if they all contain the same sub-string.
| @@ -16,6 +18,8 @@ public class SensorSetup extends RawTrackerHitSensorSetup { | |||
|
|
|||
| /// Name of the collection of fitted hits | |||
| private String fittedHitColName_ = ""; | |||
|
|
|||
| private List<String> sensorNames_ = new ArrayList<String>(); | |||
There was a problem hiding this comment.
Change this to a HashSet. You don't need to initialize the array since you do so in the setSkipSensors method below.
| @@ -29,6 +33,10 @@ public SensorSetup() { } | |||
| */ | |||
| public void setFittedHitCollection(String fittedHitColName) { fittedHitColName_ = fittedHitColName; } | |||
|
|
|||
| public void setSkipSensors(String[] sensorNames) { | |||
| this.sensorNames_ = new ArrayList<String>(Arrays.asList(sensorNames)); | |||
There was a problem hiding this comment.
Change this to Hash Map.
On a separate note, I wonder if this method should check if sensorNames was already initializing before doing it again. I can see a situation where this gets set twice on accident in a steering file and could lead to unexpected behavior. Maybe if sensorNames has already been initialized, throw an exception?
There was a problem hiding this comment.
I think the framework already checks you only set a specific variable in the steering file once and throws an error if it is there more than once.
There was a problem hiding this comment.
Given the behavior we would like from this code, not clear to me HashMap really buys us anything.
Title says all.