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.

Sign up for GitHub

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

Add a steering file for monitoring using KF tracks and add a tool to remove hits from pattern reco in LCIO #935

Merged
merged 3 commits into from
Nov 28, 2022
Merged

Add a steering file for monitoring using KF tracks and add a tool to remove hits from pattern reco in LCIO #935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4984147
Alignment monitoring steering file using KF tracks
pbutti Nov 1, 2022
a8a9767
Add removal of hits from LCIO in path finding
pbutti Nov 18, 2022
7895d00
HPS Detector as Designed
pbutti Nov 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10,070 changes: 10,070 additions & 0 deletions detector-data/detectors/HPS_TimDesign_iter0/HPS_TimDesign_iter0.lcdd
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions detector-data/detectors/HPS_TimDesign_iter0/SamplingFractions/Ecal.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
samplingFraction: 1.0
1 change: 1 addition & 0 deletions detector-data/detectors/HPS_TimDesign_iter0/SamplingFractions/Hodoscope.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
samplingFraction: 1.0
1,139 changes: 1,139 additions & 0 deletions detector-data/detectors/HPS_TimDesign_iter0/compact.xml
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions detector-data/detectors/HPS_TimDesign_iter0/detector.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
name: HPS_TimDesign_iter0
711 changes: 711 additions & 0 deletions steering-files/src/main/resources/org/hps/steering/recon/AlignmentMonitoringKF.lcsim
View file
Open in desktop

Large diffs are not rendered by default.

27 changes: 27 additions & 0 deletions tracking/src/main/java/org/hps/recon/tracking/SensorSetup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package org.hps.recon.tracking;

import java.util.List;
import java.util.ArrayList;
import java.util.Arrays;

import org.lcsim.detector.tracker.silicon.SiSensor;
import org.lcsim.event.EventHeader;
Expand All @@ -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>();
Copy link
Collaborator

@omar-moreno omar-moreno Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Change this to a HashSet. You don't need to initialize the array since you do so in the setSkipSensors method below.


/// Constructor
public SensorSetup() { }
Expand All @@ -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));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the behavior we would like from this code, not clear to me HashMap really buys us anything.

}

@Override
protected void process(EventHeader event) {

Expand All @@ -50,7 +58,26 @@ protected void process(EventHeader event) {
public void loadFittedHits(List< LCRelation > fittedHits) {

for (LCRelation fittedHit : fittedHits) {

Boolean skipHit = false;
RawTrackerHit rawHit = FittedRawTrackerHit.getRawTrackerHit(fittedHit);

SiSensor sensor = (SiSensor) rawHit.getDetectorElement();
String name = sensor.getName();

if (sensorNames_.size() > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check is unnecessary. If sensorNames_ is empty, the whole for loop will be skipped.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

for (String sensorName : sensorNames_) {
if (name.contains(sensorName)) {
skipHit = true;
break;
}
}
}

if (skipHit)
continue;

((SiSensor) rawHit.getDetectorElement()).getReadout().addHit(fittedHit);
}
}
Expand Down