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

Phase2 hps tau 13 3 #1175

Conversation

chnielse
Copy link

PR description:

This PR adds the Phase-2 HPS Tau emulator - this is the updated, firmware-based version of the Phase2L1Taus package.
The emulator uses a modified version of the HPS (Hadron-plus-strips) algorithm to identify and reconstruct tau candidates for the L1 Trigger, and outputs a standard vector of PFTau objects.

PR validation:

Basic validation has been done with standard commands:
scram build code-checks and scram build code format
with no warnings or errors due to this code.

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -j 8

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 code-checks && scram b -k -j 8 code-checks

I found no issues with the headers!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 check-headers

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no files with code format issues!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 code-format-all

@aloeliger aloeliger added Phase-2 Pertains to phase-2 development Emulator Development Emulator development PR labels Nov 8, 2023
Comment on lines 28 to 58
class Particle {
public:
pt_t hwPt;
etaphi_t hwEta;
etaphi_t hwPhi;
type_t hwId;
dz_t hwZ0;

Particle() : hwPt(0), hwEta(0), hwPhi(0), hwId(0), hwZ0() {}
Particle(pt_t pt, etaphi_t eta, etaphi_t phi, type_t id, dz_t z0)
: hwPt(pt), hwEta(eta), hwPhi(phi), hwId(id), hwZ0(z0) {}

pt_t pt() const { return hwPt; }

etaphi_t phi() const { return hwPhi; }

etaphi_t eta() const { return hwEta; }
type_t id() const { return hwId; }
dz_t z0() const { return hwZ0; }
};

//template classes
class Tau : public Particle {
public:
Tau(pt_t pt, etaphi_t eta, etaphi_t phi, type_t id, dz_t z0) : Particle(pt, eta, phi, id, z0) {}
ap_uint<5> seed_index;
pt_t seed_pt;
etaphi_t seed_eta;
etaphi_t seed_phi;
dz_t seed_z0;
};

Choose a reason for hiding this comment

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

Have you checked if there are any existing particle candidates in the data formats that you could use here instead? We have a lot of hardware representations of different particles so I would wonder if this is truly necessary.

Copy link
Author

Choose a reason for hiding this comment

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

After looking into it, there are existing options in the dataformats that I can use - currently switching to these and making sure outputs are unchanged, and will update once that's done.

Comment on lines 61 to 62
static const detaphi_t TWOPI = 3.14159 * 2. * etaphi_base;
static const detaphi_t PI = 3.14159 * etaphi_base;

Choose a reason for hiding this comment

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

Silly question, but is there some reason we use pi to this precision instead of some standard library representation?


//adding collection
std::vector<edm::Ptr<l1t::PFCandidate>> particles;
for(unsigned i = 0; i < (*l1PFCandidates).size(); i++) {

Choose a reason for hiding this comment

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

Mayeb this is a stylistic thing on my part, but does edm::Ptr not allow element access with ->?

Copy link
Author

Choose a reason for hiding this comment

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

Using -> here gives an error during compilation, so probably best left as is.


}

L1HPSPFTauProducer::~L1HPSPFTauProducer() {}

Choose a reason for hiding this comment

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

Please remove this empty function and set the destructor to default in the class definition

@aloeliger
Copy link

@chnielse Any updates on responses to PR review? I see a few changes made, any comments on the other items?

@aloeliger aloeliger added the Physics Affecting A PR expected to affect Physics content of the trigger label Nov 30, 2023
@aloeliger
Copy link

@chnielse Just checking in. Any other planned changes to this PR?

@aloeliger
Copy link

@chnielse Are all changes to this PR done from your side?

@chnielse
Copy link
Author

I'm only planning to add converting the tau output to gt format, as that isn't currently there - I should have that done today. Otherwise it's done.

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -j 8

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 code-checks && scram b -k -j 8 code-checks

I found no issues with the headers!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 check-headers

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 1 files that did not meet formatting requirements:

  • L1Trigger/L1TTrackMatch/plugins/L1TrackJetClustering.h

Please run scram b code-format to auto-apply code formatting

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 code-format-all

@chnielse
Copy link
Author

This isn't one of the files I changed/added, so I don't know why it's returning this now and not before. Not sure if I need to do anything for this?

@epalencia
Copy link

No, nothing to do from your side. It is ok.

@aloeliger
Copy link

@chnielse I think at this point this PR is ready to go to main CMSSW. @artlbv, just a heads up on this as this seems like something you would like to be aware of. @epalencia Any further things from your side before we move towards finalizing this?

@chnielse
Copy link
Author

Should be ready to go from my side.

@artlbv
Copy link

artlbv commented Jan 29, 2024

thanks, also fyi @EmyrClement

@epalencia
Copy link

@chnielse, nothing from my side, please go ahead and open the PR in master

@epalencia
Copy link

@chnielse, reminder, could you open the PR in master? Usually, it takes some time to have them reviewed by all parties

@aloeliger aloeliger changed the base branch from phase2-l1t-integration-13_3_0_pre3 to phase2-l1t-integration-14_0_0_pre3 February 5, 2024 15:37
@aloeliger
Copy link

@chnielse In order to start constructing a protoytype branch for the menu team to validate, this PR is being merged immediately to the IB.

@aloeliger aloeliger merged commit 175840a into cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3 Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Emulator Development Emulator development PR Phase-2 Pertains to phase-2 development Physics Affecting A PR expected to affect Physics content of the trigger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants