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

Synching ALERT fork with main branch #378

Open
wants to merge 90 commits into
base: development
Choose a base branch
from

Conversation

mpaolone
Copy link
Contributor

@mpaolone mpaolone commented Dec 2, 2024

No description provided.

efuchey and others added 30 commits February 15, 2024 20:10
It should have been AHDC::adc (since corresponding HitReader also uses this bank), it was AHDC::tdc.
(and stop changing wire number internally to start from zero)
(this requires removing a manual +1 offset for wire number in a perl script in GEMC)
Fix of a merge error after commit f10c09e
PreCluster pairing routine in AHDC ClusterFinder
…r-layer list of hits in AHDC PreCluster

Modified the hardcoded lookup superlayer indices to obtain the geometry parameters to calculate x, y.
Modifications for debugging/validation of index convention change.
efuchey and others added 3 commits December 3, 2024 13:54
…ector_simple" for Hit, and "H_simple", "h_simple" for KFitter to return the preexisting matrix emasurements;

new matrix measurements have been implemented in the methods "getMeasurementNoise", "getVector", "H" and "h".
Added new memebers "adc" and "phi" for the hit for measured ADC and wire phi position at z = 0 respectively.
Getting changes from Michael
@@ -27,7 +27,9 @@ public Cluster(PreCluster precluster, PreCluster other_precluster) {
_PreClusters_list.add(precluster);
_PreClusters_list.add(other_precluster);
this._Radius = (precluster.get_Radius() + other_precluster.get_Radius()) / 2;
this._Z = ((other_precluster.get_Phi() - precluster.get_Phi()) / (Math.toRadians(20) * Math.pow(-1, precluster.get_Super_layer()) - Math.toRadians(20) * Math.pow(-1, other_precluster.get_Super_layer()))) * 300 - 150;

this._Z = ((other_precluster.get_Phi() - precluster.get_Phi()) / (Math.toRadians(20) * Math.pow(-1, precluster.get_Super_layer()-1) - Math.toRadians(20) * Math.pow(-1, other_precluster.get_Super_layer()-1))) * 300 - 150;
Copy link
Collaborator

Choose a reason for hiding this comment

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

hardcoded numbers should be replaced with variables possibly defined from the database or in this case the geometry service

if (precluster.get_Super_layer() == super_layer && precluster.get_Layer() == layer && !precluster.is_Used()) {
ArrayList<PreCluster> possible_precluster_list = new ArrayList<>();

double phi_mean = precluster.get_Phi() + 0.1 * Math.pow(-1, precluster.get_Super_layer());
double phi_mean = precluster.get_Phi() + 0.1 * Math.pow(-1, precluster.get_Super_layer()-1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment about hardcoded numbers

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 it's the "0.1" in question

switch (this.superLayerId) {
case 0:
case 1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment about hardcoded numbers

Copy link
Collaborator

Choose a reason for hiding this comment

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

comments in the ATOF::add bank should be updated to be consistent with sector/layer starting from 1

etc/bankdefs/hipo4/data.json Outdated Show resolved Hide resolved
@@ -23,21 +23,21 @@ private void fill_list(List<Hit> AHDC_hits, ArrayList<Hit> sxlx, int super_layer

public void findPreCluster(List<Hit> AHDC_hits) {
ArrayList<Hit> s0l0 = new ArrayList<Hit>();
fill_list(AHDC_hits, s0l0, 0, 0);
fill_list(AHDC_hits, s0l0, 1, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The List of Lists should be filled all at once to avoid looping 12 times through the hits

public int id;

public float adcMax;
public float timeRiseCFA;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this class is for general use, members' names should be more generic removing the CFA or CFD suffix.

Also the pulse "rise" and "fall" are usually referred to as leading and trailing edge that could be better names

public long timeStamp;
public float fineTimeStampResolution;
public static final short ADC_LIMIT = 4095; // 2^12-1
public float amplitudeFractionCFA;
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the pulse specific variables should be local variables in the extract method

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, good catch, critical for thread safety

@baltzell baltzell mentioned this pull request Dec 6, 2024
efuchey and others added 3 commits December 9, 2024 10:34
…tem, one to minimize track distance.

Added functions to calculate phi for hit as a function of z and to calculate the hit "sign" i.e. the side of the wire the track passes.
…ers all hits to make a track candidate and uncommented the track finding code.
Kalman Filter fixes: constants and parameters
@baltzell
Copy link
Collaborator

baltzell commented Dec 10, 2024

And the creation of BMT::wf (and conversion to BMT::adc) in the decoder was intended only as a working example, to be replaced with AHDC.

@baltzell
Copy link
Collaborator

Ok, after this is merged, then the Mode3 class in the decoder needs to be switched to the one AHDC wants to use.

@baltzell
Copy link
Collaborator

baltzell commented Dec 13, 2024

And this will address that and that (and create the still missing AHDC::wf bank definition)

mpaolone and others added 2 commits December 16, 2024 08:59
Actually use the new waveform conversion approach for AHDC
Use DeepJavaLibrary to load a PyTorch model.
Create a new bank AHDC_AI::Prediction.
Copy link
Collaborator

@whit2333 whit2333 left a comment

Choose a reason for hiding this comment

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

ATOF geometry should be untouched.

@baltzell
Copy link
Collaborator

This is now required.

Revert "leave internal indexing at zero, output at 1"
@baltzell
Copy link
Collaborator

Who can resolve the ATOF geometry conflict?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants