Tau L1 2023 #1087
Tau L1 2023 #1087
Conversation
|
Hi @Duchstf , could you please a description for this PR? |
aloeliger
added
Phase-2
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. Attempts to compile this PR failed. The following is the stderr of the compilation attempt:
|
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. This PR failed the code checks. I found the following lines where an "error" was mentioned, they may help in debugging Please check and see if these lines help debugging.
I found no issues with the headers!| Info | Value | |
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found 1 files that did not meet formatting requirements:
Please run |
|
@Duchstf Could you please fix the compilation errors in this PR? |
|
@Duchstf can you specify which of the four changes you mention are already included here, and which are to come? For your second point, if needed would we be able to replicate the performance of the new NN ID without also migrating to the new hls4ml library? |
|
@Duchstf Out of curiosity is this trying to run an existing tensorflow graph NN in CMSSW? Is this trying to implement an hls4ml model in CMSSW? |
|
@Duchstf Pinging you again on the compilation issues and code format. |
|
@aloeliger @EmyrClement Hello, thank you both for the pings and sorry for the late reply!! I pushed the necessary changes! Let me know if I need to fix anything! Note that the HW emulation is still not updated with the new training so you might want to use the SW emulation version for now. I'll look into replacing the weights in the HW emulated version and hopefully gets it merged soon as well. Thanks, Duc. |
|
@Duchstf Thank you very much. One question: do the performance plots we saw yesterday corresponds to HW flag + new training? Then, I understood that the HW flag is not so important for the performance, but I think it's pretty needed expecially now that we need to use the "emulator-matching-firmware" in the GT emulator for HLT (together with updating the GT format as discussed yesterday). Could you please discuss in today's software meeting if the HW emulated version (and the GT format) can arrive soon enough for the Menu/GT team? @EmyrClement @mcepeda |
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. Attempts to compile this PR succeeded!
|
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found no issues with the code checks!
I found no issues with the headers!
|
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found 5 files that did not meet formatting requirements:
Please run
|
|
@Duchstf Could you please fix the file formatting on this? |
|
@Duchstf I'm confused with something - in your latest commits you update a As @cbotta said, it would be good to confirm that the plots you showed yesterday can be reproduced with the changes you are proposing in this PR. |
|
Ahh, thank you for reminding me @EmyrClement. Committing model files, data files and the like is not acceptable in CMSSW anymore. The file: L1Trigger/Phase2L1ParticleFlow/data/modelTT_PUP_Off_dXY_XYCut_Graph.pb needs to be removed from this commit, and removed from the git history. It will need to be committed separately to the data files. |
|
Just to clarify, that file was updated here, not added. And in any case, it shouldn't be touched by this PR - let's remove the change from the history. I expect some model files should be updated though - given the tight timescale, do we update here for now to have them available, and then migrate them to cms-data when we make a PR to central? |
|
Ahh, you are quite right. I am dealing with the removal of this very file in CMSSW right now from the correlator updates. Okay. In any case, let us not make a problem worse, and please discard any changes to this file in this commit. My concern with committing files here is that it will prevent synchronization with central CMSSW. If we need to perform PRs, as in the correlator PR right now, it also falls us on to remove them from the history. If they are necessary, they need to go in the data externals as soon as possible. If they are inextricable from trigger function, we should come up with a better solution than their storage here. |
|
Hi all, I updated the HW weights, replaced the protobuf file and updated the file formatting. Duc. |
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. Attempts to compile this PR failed. The following is the stderr of the compilation attempt:
|
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. This PR failed the code checks. I found the following lines where an "error" was mentioned, they may help in debugging Please check and see if these lines help debugging.
I found no issues with the headers!
|
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found 7 files that did not meet formatting requirements:
Please run
|
|
@Duchstf Thanks for the latest update! As you can see from the tests there are compilation errors and formatting requirements that need fixing. I had some questions related to those compilation errors, which I'll add inline to the code. |
|
I also tried testing the simulation by setting Did it work for you? |
| lSeed = l1t::PFCandidate::Photon; | ||
| std::copy_if(work.begin(), work.end(), std::back_inserter(seeds), [&](const l1t::PFCandidate& part) { | ||
| return ((part.id() == l1t::PFCandidate::Electron || part.id() == l1t::PFCandidate::ChargedHadron || | ||
| part.id() == lSeed) && |
There was a problem hiding this comment.
This logic looks ok, but it's a bit confusing so want to make sure there isn't a bug. When fEMSeed is false, this line becomes (part.id() == l1t::PFCandidate::Electron || part.id() == l1t::PFCandidate::ChargedHadron || part.id() == l1t::PFCandidate::ChargedHadron) i.e. the last conditional is repeated. Is that intended?
There was a problem hiding this comment.
@Duchstf can you check this comment?
|
@EmyrClement @aloeliger Hello, I squashed the comments but I'm a bit lost as to which branch of central cmssw I should make the PR to ... could you maybe provide me some pointers on this? |
|
@Duchstf I still see this as 21 commits. Please squash the commits in this PR (If you haven't done this before, please back up this branch with a duplicate before you do anything, just so it can be recovered if necessary). Once you have this PR as 1 commit, please open a PR to the main CMSSW master branch. You can do this by getting the latest CMSSW 13_1_0 release (currently CMSSW_13_1_0_pre3), and using git cherry pick to move the single commit to that branch. From there, open a PR to the branch |
|
Hi Andrew, I put in the PR here: cms-sw#41492 However, I'm having troubles to even compile it locally, and the errors seem to be not related to my changes: I'm wondering if anyone have any information on this. Or is there a separate set of instructions I need to follow in order to set things up for the cmssw master branch? |
Hi, It looks like your error seems to appear in a directory for the 12_5_2 release, but the main CMSSW branch is targeting 13_1_X if I remember correctly (Andrew, please correct me if I'm wrong). That might explain why you get that error when trying to compile locally. I suspect you have the L1TNtuple directory checked out locally with the changes from 13_1_X while the DataFormats/L1TMuon directory isn't checked out and therefore retrieved from the 12_5_2 release where the update of the muon showers hasn't happened yet.. Cheers, |
|
@Duchstf do you need any help from us to address the issues with the PR to master? On another topic, I was looking at these lines again. I may have misunderstood something before, and need you to double check these are correct. Looking at this line: I previously said that is then wrong, and should be: or, depending on what's done in the firmware, just: And similar for phi. Can you check this for me? |
|
Hi @EmyrClement, sorry for the late reply! I've been busy with finals!! I guess we could carefully go through each function/dataypes definition carefully here: Where which I think it's a digitized quantity for eta as well. And we know that So I personally think what we are doing here is correct ... Let me know what you think!! Duc. |
|
Hi @Duchstf sorry for taking long to reply. Are you sure Experimentally, looking at a few candidates I also see |
|
@EmyrClement I did some printouts and I think you are right: (Converted eta is the eta we are using, and it seems like it goes above 2.5 sometimes), so I think it's correct that |
|
@Duchstf, could you have a look at the existing conflict? |
|
@Duchstf, now that cms-sw#41492 is merged, could you please include here the commits added in master? |
|
@epalencia Yes, sorry I'm checking to make sure what changes I should make here, since we included a fix for the newer version of CMSSW, but I'm not sure if that change would be compatible here. |
|
Hello, I pulled the latest changes from phase2-l1t-integration-1252patch1, and I'm getting this error when trying to compile it: I don't think this is caused by my code, does anyone know what's going on here? |
|
@Duchstf @epalencia @aloeliger If I understand correctly, Duc has pulled over the additional commits made to master on top of this PR? The comment made by Duc was from trying to rebase to the tip of phase2-l1t-integration-1252patch1, but I don't think this needs to be done, and will be handled by the merge (and checked by triggerDoctor)? The only other issue to resolve is with the TauNNCache, which we said would be best followed up in a separate PR. |
|
@Duchstf Looking at this again, I reproduced your problem, which can be fixed in my local checkout by doing: and rebuilding. Can you try this and report back? |
|
@EmyrClement Hello, I compiled it and everything looks fine! Sorry for the late response. |
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. Attempts to compile this PR succeeded!
|
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found no issues with the code checks!
I found no issues with the headers!
|
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found 1 files that did not meet formatting requirements:
Please run
|
|
Backport of cms-sw#41492. |
|
Tagged as |
PR description:
This is a PR to update the TauNN with the latest training. The changes would include:
Related materials:
I'll keep updating this overview as the PR moves along!