-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
mkFit external integration #36546
mkFit external integration #36546
Conversation
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36546/27470 ERROR: Build errors found during clang-tidy run.
|
Can you clarify? Should this be in the draft state (in which case, please change it), or should we consider it "ready for review"? |
[not with my reco hat] There was a push to have this code in a PR as soon as reasonably possible. Perhaps this is a better way to converge anyways. |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36546/27499
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36546/27530
|
A new Pull Request was created by @osschar (Matevž Tadel) for master. It involves the following packages:
The following packages do not have a category, yet: RecoTracker/MkFitCMS @jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
enable profiling |
@cmsbuild please test |
-1 Failed Tests: Build BuildI found compilation warning when building: See details on the summary page. |
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.
Just a few suggestions for possible cleanup.
Not meant for this PR, but you can perhaps include them in #36966
int lOffset = 0; | ||
if (lo_ == TkLayout::phase1) | ||
lOffset = 1; |
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.
This can move into the scope that starts after L97 (even better: after L100)
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 decided to refactor this part of the code in connection to other comments (it will come together with the update for the magic numbers in the layer index literals)
#ifndef RecoTracker_MkFitCMS_interface_buildtestMPlex_h | ||
#define RecoTracker_MkFitCMS_interface_buildtestMPlex_h |
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.
#ifndef RecoTracker_MkFitCMS_interface_buildtestMPlex_h | |
#define RecoTracker_MkFitCMS_interface_buildtestMPlex_h | |
#ifndef RecoTracker_MkFitCMS_standalone_buildtestMPlex_h | |
#define RecoTracker_MkFitCMS_standalone_buildtestMPlex_h |
#include "RecoTracker/MkFitCore/interface/Config.h" | ||
#include "RecoTracker/MkFitCore/interface/Track.h" | ||
|
||
#include "nlohmann/json.hpp" |
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.
#include "nlohmann/json.hpp" | |
#include <nlohmann/json.hpp> |
I am a bit worried by the large number of hardcoded numerical parameters in several files included in these packages, and I wonder how can this code be easily maintainable and scalable... |
It would help to be a bit more specific if the comment is anticipating some action. I'm not sure I understand the note about scalability. which dimensions are you considering? I suppose it should still be in the context of the CMS design. Specific to the geometry(geometries), the current implementation is applicable to the phase-1 geometry and would work for 2017 just about as well as for the anticipated 2024 conditions. Please clarify. |
@slava77 my comment did not require any action, sorry for not having specified it clearly. Simply, when I (finally) went through the whole code I got the impression (apparently shared by other commenters that posted before me) that there are quite a lot of hardcoded parameters here and there in the code, whose meaning can only be clear to the authors of the code themselves or to someone who becomes expert with it. What happens if some special condition happens during the run, can it force us to tune them differently? You are claiming here that "the current implementation is applicable to the phase-1 geometry and would work for 2017 just about as well as for the anticipated 2024 conditions" which is already a good test of robustness, indeed. On the other hand, it is quite common to the Tracking code to have parts which can only be fully understood by the real experts, and hard to maintain or intervene by other less experienced developers: we had plenty of examples as such also in the past. If you want, my comment could be rephrased propositively as: when you will review and clean the code after this PR had been merged, please take care of the parts (mostly already commented specifically in this github thread) where there are "obscure" hardcoded parameters: perhaps giving in a few case some meaningful name, or adding some comment line, can help possible future maintenance of the code. |
@perrotta I noted your comments in #36546 (review) and would prefer to address them in the follow up PR(s). Based on your comment I conclude that you went through the code by now. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-79c073/22526/summary.html The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Comparison SummarySummary:
|
+1 |
This PR broke
I'd guess other non-x86 IBs would fail to build in similar way. |
I guess something like https://github.com/cms-sw/cmsdist/blob/f2fd150c7f36845f4f5a00a60b24dd33d265ec7b/mkfit-2.0.0-non-x86_64-fix.patch |
as patch was applied for non x86_64 so how about
|
I'm curious if there is perhaps a feature-specific macro. |
looking at https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i386/immintrin.h#L24 but this seems ugly, or is it OK? |
nevermind, that's a tautology |
PR description
Integrate mkFit into CMSSW from external package.
This PR adds two subpackages to RecoTracker: MkFitCore and MkFitCMS – see below for their overall structure and content. Both of them include a subdirectory “standalone/” where code that is not needed for operation within CMSSW was collected. However, some of this code will need to be reactivated for CMSSW as it produces auto-generated code that describes Phase1 geometry and mkFit track-finding parameters.
PR validation
MTV results in TTbar MC with PU (Run3, 2021 realistic conditions), comparing performance before and after mkFit integration in CMSSW:
http://uaf-10.t2.ucsd.edu/~mmasciov/MTV_TTbarPU_cmsswIntegration/