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

Spurious comparison differences in workflows 136.X, 141.044, 4.54, ... #43456

Closed
perrotta opened this issue Nov 30, 2023 · 12 comments
Closed

Spurious comparison differences in workflows 136.X, 141.044, 4.54, ... #43456

perrotta opened this issue Nov 30, 2023 · 12 comments

Comments

@perrotta
Copy link
Contributor

Since a few IB's the workflows in the title show spurious comparison differences in DQM tests.
There are 6 failing comparisons in ParticleFlow_JetResponse_slimmedJets and 6 failing comparisons in ParticleFlow_JetResponse_slimmedJetsPuppi, both in the JEC subfolder.

Examples in:

Well, all PRs I can see that were run with the latest IB's show this "feature", while I don't see it for PRs tested with CMSSW_14_0_X_2023-11-28-1100 (e.g.)

The PRs that were merged between CMSSW_14_0_X_2023-11-28-1100 and CMSSW_14_0_X_2023-11-29-1100 are:
#43404 from smuzaffar: [SIMULATION] [DEVEL] Fix warnings on deprecated copy-constructors
#43338 from fwyzard: Extend the GenericConsumer to consume individual products
#43403 from civanch: [14_0_X SIM] Update FTFP_BERT_EMN physics
#43339 from bouchamaouihichem: Enabling the JetCore+DeepCore2.2.1 hybrid with MaxCand=30 (JC30DC)
#43242 from francescobrivio: Read VertexSmearing from GT in Run-1/2/3 MC
#42936 from fwyzard: Fix logic error in FinalPath checks
#43164 from RSalvatico: Change EGM code to be able to read HCAL PF thresholds from DB
#43386 from mmusich: [TkAl] remove "old" all-in-one metatool from cmssw
#43385 from mmusich: fix HLT track collection for SiStrip at HLT monitoring
#43348 from ggonzr: Update hash_name function for RelMon utilities
#43176 from swagata87: Migrate RecHit-based rho producer to use HCal noise cuts from GT
#43410 from aandvalenzuela: [ALCA][DB] Fix copy-constructor warnings reported in DEVEL IBs
#43304 from peteryouBuffalo: UPDATED: Adding of jet purity and efficiency plots to PFValidation package

The last PR (#43304) at a first glance could be the responsible...,

@perrotta
Copy link
Contributor Author

assign pf, dqm

@cmsbuild
Copy link
Contributor

New categories assigned: pf,dqm

@rvenditti,@syuvivida,@tjavaid,@nothingface0,@antoniovagnerini,@bellan,@kdlong,@swagata87 you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

A new Issue was created by @perrotta Andrea Perrotta.

@Dr15Jones, @smuzaffar, @antoniovilela, @sextonkennedy, @rappoccio, @makortel can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@civanch
Copy link
Contributor

civanch commented Dec 1, 2023

assign simulation

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2023

New categories assigned: simulation

@civanch,@mdhildreth you have been requested to review this Pull request/Issue and eventually sign? Thanks

@swagata87
Copy link
Contributor

FYI @cms-sw/jetmet-pog-l2

@makortel
Copy link
Contributor

makortel commented Dec 1, 2023

@civanch Why assign to simulation? On a quick look only(? or mainly?) data workflows are affected.

@civanch
Copy link
Contributor

civanch commented Dec 1, 2023

@makortel , in the list of PRs SIM present, likely has nothing to do with the problem. I mainly add "simulation" for fast access of the issue.

@mmusich
Copy link
Contributor

mmusich commented Dec 2, 2023

The last PR (#43304) at a first glance could be the responsible...,

What about #43304 (comment) ?

@perrotta
Copy link
Contributor Author

perrotta commented Dec 8, 2023

@peteryouBuffalo in #43304 (review) was pointed out that there is an issue with the case if nEvents == 0.
In that thread you wrote "I will try to reach out and discuss with my colleagues at Buffalo who originally started and helped with the original PR (#39902), so we can figure out resolving this specific case. My assumption is that it should be fine to just continue if nEvents == 0, but I will check with others to confirm." (which I report here for completeness).

This issue is becoming annoying, and it can make difficult to check and review new PR. Did you verify whether it is correct to just continue after L126 if nEvents = 0? At the first order, I would just verify that it is not disrupting anything, as it make sense not continuing there if there are no events.

Could you please let us know if a PR is coming? Otherwise we can just try that suggestion in a fix PR and see what happens...

@peteryouBuffalo
Copy link
Contributor

@perrotta We have a PR coming to fix this issue. @laurenhay is going to make a PR to add a statement to fix the nEvents == 0 case. We will have the PR taken care of as soon as possible.

@perrotta
Copy link
Contributor Author

#43530 fixed the issue of the weird behaviour when nEvents = 0

It looks like it also fixed what pointed out here, that is the spurious differences in the test outputs of several workflows.

In fact, the PRs tested after #43530 was merged (since CMSSW_14_0_X_2023-12-11-2300) do not show those differences in the test output any more, see for example #43544 or #42633

Therefore, I think that this issue can get closed now

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

No branches or pull requests

7 participants