Skip to content
This repository has been archived by the owner on Dec 9, 2024. It is now read-only.

Remove cppitertools + Small Cleanup #376

Merged
merged 11 commits into from
Mar 21, 2024
Merged

Remove cppitertools + Small Cleanup #376

merged 11 commits into from
Mar 21, 2024

Conversation

GNiendorf
Copy link
Member

@GNiendorf GNiendorf commented Mar 13, 2024

Small cleanup PR that removes cppitertools and unused/duplicate files. I noticed that cppitertools was only used in trkCore.cc and imported but unused in a bunch of other places and figured I would remove it and get rid of the directory.

@GNiendorf GNiendorf changed the title Remove cppitertools Remove cppitertools + Small Cleanup Mar 14, 2024
@GNiendorf
Copy link
Member Author

My last commit gets rid of the unused helper.cc/h and renames helper_v2.cc/h to just helper.cc/h

@GNiendorf
Copy link
Member Author

Last commit makes pt cut configurable with --pt_cut flag for lst_plot_performance.py and -p flag for createPerfNumDenHists.

@GNiendorf
Copy link
Member Author

/run standalone
/run cmssw

@GNiendorf GNiendorf requested review from sgnoohc and slava77 March 15, 2024 18:40
Copy link

There was a problem while building and running in standalone mode. The logs can be found here.

@GNiendorf
Copy link
Member Author

@ariostas Not sure how to coordinate this, but after this PR is merged I think you'll have to update your CI a bit: Getting this error on the standalone "rm: cannot remove 'src/helper_v2.o': No such file or directory" since I renamed helper_v2 to just helper and deleted the old helper.cc that was just an outdated version of the _v2

@ariostas
Copy link
Member

ariostas commented Mar 15, 2024

@GNiendorf Yeah, I'll change it so that it runs make clean || echo "skip" so that the CI doesn't stop if it doesn't delete something.

The CMSSW check still doesn't work since I haven't updated the step2 file, but hopefully in a couple of hours it should work.

Copy link

There was a problem while building and running with CMSSW. The logs can be found here.

@GNiendorf
Copy link
Member Author

Last commit is an attempt at fixing issue #306, removing as much of the remaining legacy CPU code as I can and moving only what's used in the GNN output filling to SDLMath.h. @jkguiang Can you check the GNN ntuple to confirm there are no changes when you get a chance? Thank you.

@GNiendorf GNiendorf linked an issue Mar 15, 2024 that may be closed by this pull request
@GNiendorf
Copy link
Member Author

GNiendorf commented Mar 16, 2024

It looks like make_classfiles is a duplicate of this code here: https://github.com/cmstas/Software/blob/739fe8d21e76d52623f8ff70e68c2f17ae923176/makeCMS3ClassFiles/make_classfiles.py

Edit: I noticed trktree.cc also has this same format, probably generated from make_classfiles.py. With that in mind I think it's good to keep it in the repo, even though it is pulled from above.

@GNiendorf
Copy link
Member Author

I think this PR is ready for review when you get a chance @slava77.

@slava77
Copy link
Contributor

slava77 commented Mar 16, 2024

/run standalone
/run cmssw

Copy link

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

Copy link

There was a problem while building and running with CMSSW. The logs can be found here.

Copy link

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

@GNiendorf
Copy link
Member Author

I think the variation in the CMSSW plots is because my other PR #374 was merged in recently, so this is really showing the change due to that PR.

@GNiendorf
Copy link
Member Author

/run cmssw

Copy link

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks
Efficiency and fake rate vs pT, eta, and phi

The full set of validation and comparison plots can be found here.

@sgnoohc
Copy link
Contributor

sgnoohc commented Mar 19, 2024

i don't see any problematic things. so it LGTM.

Copy link
Contributor

@slava77 slava77 left a comment

Choose a reason for hiding this comment

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

there are a couple of simple updates that could be done now.
although with some good arguments could be done in a followup PR.

code/core/SDLMath.h Outdated Show resolved Hide resolved
efficiency/src/helper.cc Show resolved Hide resolved
@GNiendorf
Copy link
Member Author

Last commit fixes known issue where the y-axis would be cut off sometimes in comparison plots. Left is before, right is after my last commit.

TC_fakerate_etacoarsezoom-1 TC_fakerate_etacoarsezoom

@GNiendorf
Copy link
Member Author

@slava77 This PR should be good to go if you don't have any comments about my last 2 commits.

@slava77
Copy link
Contributor

slava77 commented Mar 21, 2024

/run standalone

Copy link

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

@slava77 slava77 merged commit f4a2fdd into master Mar 21, 2024
2 checks passed
@GNiendorf GNiendorf deleted the remove_iter branch March 26, 2024 14:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Remaining Legacy CPU Code After Alpaka Move
4 participants