-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add LST and LSTCore #30
base: master
Are you sure you want to change the base?
Conversation
This is a replacement of #29. This is still a draft since it includes all the |
Will check it out tonight and maybe test it and report back. Thanks! |
these will likely trigger an issue of increasing the git repo by too much and will need to be removed from the git history. Is it a matter of getting CI to know where to take the data files? |
f173c6b
to
8ced0f1
Compare
Yeah, I removed them from the git history
Yeah, I'll do that. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I foresee a comment at the review about the many commits with "invalid states", e.g. when the LST was included as an external package, something that was/will never be useful for CMSSW. Based on this, shall we squash the commits? Maybe two commits would make sense, the one that Added LSTCore and one with the rest of the commits in the cmssw repo.
else | ||
export SCRAM_ARCH=el8_amd64_gcc12 | ||
fi | ||
export CMSSW_VERSION=CMSSW_14_1_0_pre3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ok if we leave it like that? Maybe we should just grab the version of CMSSW we have set up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a standalone setup, somewhat tuned to available input file formats and other dependencies (like a test file format).
Also, aren't the following lines (from cmsrel) implying that the version is not known otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a better setup script. I only made the minimal modifications to get it to work.
I also tried the standalone version within CMSSW. It compiles fine but it doesn't run because:
I guess this is because data files were removed from the history. Are there changes pending for things to work? Or is there some manual intervention required, like I need to clone the data files with an extra command? |
we should probably decide what "standalone" would actually mean:
That's a bit related to the above:
|
and if that path is not available, to add a git clone/checkout from https://github.com/SegmentLinking/RecoTracker-LSTCore |
In principle both can work with small changes to the current state of the code, I believe. For the first case, all we need is to modify: cmssw/RecoTracker/LSTCore/src/alpaka/LSTESData.dev.cc Lines 14 to 30 in 8ced0f1
to point to a different directory according to your comment:
For the second case, I did:
Given the absence of the
Then, the include directories are a bit off, because there is no proper cmssw/RecoTracker/LSTCore/src/alpaka/Kernels.h Lines 7 to 10 in 8ced0f1
to point to the appropriate directories, as these lines are useless right now, if also the standalone is in CMSSW. UPDATE:As of 6025567, the fix of the |
@@ -0,0 +1,157 @@ | |||
#ifndef Constants_cuh | |||
#define Constants_cuh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will need compliant header guards; not sure it's an ovekill to do them with #ifdef LST_IS_CMSSW_PACKAGE
;
otherwise
#define Constants_cuh | |
#define RecoTracker_LSTCore_alpaka_Constants_cuh |
@@ -0,0 +1,7007 @@ | |||
#include "trktree.h" | |||
trktree trk; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this auto-generated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgnoohc Perhaps you know/remember?
else | ||
export SCRAM_ARCH=el8_amd64_gcc12 | ||
fi | ||
export CMSSW_VERSION=CMSSW_14_1_0_pre3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a standalone setup, somewhat tuned to available input file formats and other dependencies (like a test file format).
Also, aren't the following lines (from cmsrel) implying that the version is not known otherwise?
float drt_InSeg = rtMid - rtIn; | ||
float dz_InSeg = zMid - zIn; | ||
float dr3_InSeg = | ||
alpaka::math::sqrt(acc, rtMid * rtMid + zMid * zMid) - alpaka::math::sqrt(acc, rtIn * rtIn + zIn + zIn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: pick up the update from SegmentLinking/TrackLooper#407
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #44.
Yeah, I think it makes sense to squash commits further. Let's also resolve all the discussions here by adding more commits to this branch and once it's in good shape we'll squash everything into 1-2 commits. |
Here's a new branch with squashed commits. I can change the branch name and/or commit messages if you prefer. https://github.com/SegmentLinking/cmssw/tree/LST_and_LSTCore |
Seems good to me. Should we |
Yeah, sounds good. I wasn't sure about that so I made a separate one to be safe, but let's just stick with this branch for discussions. |
83cc530
to
891eb11
Compare
There are lots of changes after the force-push, but that's just because I re-synced with upstream master. |
the actual diffs in the https://github.com/SegmentLinking/cmssw/pull/30/files look OK. |
dc4e61f
to
7c6bcfa
Compare
Should we close this PR now? It seems like it's no longer needed for discussions. |
I think that the point of this PR is to mirror the ones in CMSSW master, so that when the central one is merged, this is also merged automatically. At least that's what I had understood based on the mkFit model of development. |
yes, that's the idea. Also, being able to run our CI here would also be helpful; although the comparisons would have to be with an earlier version of itself; so, perhaps more complicated. Would it be possible to at least run the linter part? |
Should I sync the SegmentLinking/cmssw master to the cms-sw/cmssw master? I guess that would allow us to see the conflicts also here? |
fix `SiStripBadStrip_PayloadInspector` after merging of cms-sw#45795
Updated cms::Exception using modern C++
[DQM] Changes suggested by new llvm18 clang-format
…ang-format [ALCA-UPGRADE] Changes suggested by new llvm18 clang-format
[Phase 2 L1T] fixing ASAN errors in Phase2L1CaloJetEmulator
Phase2-hgx360A Modify the V19 longitudinal structure of HGCal according to Meeting specification of 06/09/2024
rename modifier `run3_2024_L1T` to `stage2L1Trigger_2024`
Additional tracks filter for vertices (minimum no. strip hits)
Fix Alpaka Harvesting for Phase2
Reset Pixel Dead channels map Online GUI
Phase-2 GT interface update and slice test pattern writers
…g-format [ALCA-DB-L1] Changes suggested by new llvm18 clang-format
Fix mismatch between EB and EE recHits in Phase2 supercluster producer configs
…atch Better exception message if module label in ESInputTag doesn't match
…ormat [ALCA-L1] Changes suggested by new llvm18 clang-format
o2oRun_SiStripDCS email warning fix
Add 2024 HI eras to reco processing configuation
…TCore_realfiles_batch6 Batch 6 for updates to LST integration in cms-sw
…ecks/cms-sw-PR-45117/41778/code-format.patch; the result looks less readable, but we have to comply
Here's a draft of the LSTCore package, but now using real files. Here are some notes about it.
I tested running both standalone and CMSSW, and everything seems fine.