Skip to content

SUSY Run 2#574

Open
ChrisJChang wants to merge 392 commits intomasterfrom
SUSYRun2_mastermerge
Open

SUSY Run 2#574
ChrisJChang wants to merge 392 commits intomasterfrom
SUSYRun2_mastermerge

Conversation

@ChrisJChang
Copy link
Collaborator

@ChrisJChang ChrisJChang commented Jan 23, 2026

This adds the code for the SUSY Run 2 project, which includes many new analyses, and ColliderBit code.

Do not be scared by the very large number of changed files. This PR includes many spelling fixes in comments and removed trailing whitespace.

I have added a few reviewers, but this may change. Please wait to start your review until I say (I may make some more small improvements first)

Some tests for this PR that have been run (locally):

  • CBS runs on a single analysis
  • ColliderBit_CMSSM.yaml works
  • Linux build

Some tests that should be run (feel free to add to):

  • Mac build
  • Github CI runners (currently failing on master)

tprocter46 and others added 30 commits December 11, 2023 15:57
…vfb.cpp to a separate pair of hpp/cpp files.
anderkve and others added 18 commits June 26, 2025 16:19
…that are actually added to the results of a given analysis (registered in collect_results()), instead of all the SRs used in the analysis run(...) function. Also removed debug output.
…verlap system. Updated some analyses as required.
Also update SUSY Run 2 yaml files to work.
ColliderBit_CMSSM.yaml now works with contur
Fix CBS so it now runs properly
Remove a TODO comment
@Pengxuan-Zhu-Phys
Copy link

I did a local pass on PR #574 from an isolated checkout of SUSYRun2_mastermerge and I do not think this is ready for blanket approval yet.

The main PR-specific issue I found is in ColliderBit/src/analyses/Analysis_ATLAS_13TeV_2LEPsoft_139invfb.cpp: the low-pT muon preselection loop iterates over event->electrons() instead of event->muons(). That duplicates electron tracks and drops the intended muon contribution in the preselected track collection. I fixed this locally (Line 214) as:

- for (const HEPUtils::Particle* mu : event->electrons())
+ for (const HEPUtils::Particle* mu : event->muons())

@Pengxuan-Zhu-Phys
Copy link

Detailed Review Comments for PR #574

NOTE: This comment is completed with AI assistance, basically make a summary of file changes
Here is a collection of the detailed review comments for the local fixes and portability issues I found while testing the SUSYRun2_mastermerge branch on macOS with the clang-based preset.
CmakeUserPresets.json

For context, the macOS build issues I reported were reproduced locally on the following machine/configuration:

  • Hardware: Apple M4 Pro
  • Architecture: arm64 (Apple Silicon)
  • OS: macOS 26.3.1 (build 25D2128)
  • Xcode SDK: MacOSX 26.2
  • SDKROOT: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk
  • CMake: 4.1.1
  • C compiler: Homebrew clang 19.1.7
  • C++ compiler: Homebrew clang++ 19.1.7
  • Fortran compiler: GNU Fortran 15.1.0 (Homebrew GCC 15.1.0)
  • Python: 3.10.12

The comments below are written so they can be pasted directly into GitHub review comments. For each file, I have included:

  1. What the issue is.
  2. Why it matters.
  3. A ready-to-paste English review comment.

1. ColliderBit/src/analyses/Analysis_ATLAS_13TeV_2LEPsoft_139invfb.cpp

What changed locally

- for (const HEPUtils::Particle* mu : event->electrons())
+ for (const HEPUtils::Particle* mu : event->muons())

Why this matters

This is a correctness bug in the analysis code. The low-pT muon contribution to preselectedTracks is being read from the electron collection, which duplicates electron tracks and omits the intended soft-muon tracks.

Suggested review comment

This loop is filling the low-pT muon track contribution from event->electrons() rather than event->muons(). As written, electron tracks are duplicated and soft muons are omitted from preselectedTracks, which changes the track collection used by the analysis. This should iterate over event->muons() instead.

2. ColliderBit/include/gambit/ColliderBit/analyses/AnalysisMacros.hpp

What changed locally

- _cutflows.addCutflow(SR, {"Preselection", ##__VA_ARGS__, "Final"});
+ _cutflows.addCutflow(SR, {"Preselection" __VA_OPT__(,) __VA_ARGS__, "Final"});

- ADD_CUTFLOW(NAME, ##__VA_ARGS__)
+ ADD_CUTFLOW(NAME __VA_OPT__(,) __VA_ARGS__)

- DEFINE_SIGNAL_REGION(name,## __VA_ARGS__)
+ DEFINE_SIGNAL_REGION(name __VA_OPT__(,) __VA_ARGS__)

Why this matters

The original macro form relies on the GNU , ##__VA_ARGS__ extension. Under the macOS clang toolchain used for this branch, the build fails with -pedantic-errors because clang rejects that extension as -Wgnu-zero-variadic-macro-arguments.

This means the branch is not build-clean on macOS/clang even when the overall preset and toolchain are set up correctly.

Suggested review comment

This macro relies on the GNU , ##__VA_ARGS__ extension. Under the macOS clang toolchain used for this branch, -pedantic-errors rejects that form (-Wgnu-zero-variadic-macro-arguments), which blocks the build in ColliderBit. Please switch these variadic macros to a standard form such as __VA_OPT__(,) __VA_ARGS__ so the analysis macros expand cleanly on clang as well as GCC.

3. cmake/contrib.cmake

What changed locally

- CMAKE_COMMAND ${CMAKE_COMMAND} ..
- CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER} -DCMAKE_CXX_FLAGS=${HEPMC_CXX_FLAGS} -DHEPMC3_ENABLE_ROOTIO=${HEPMC3_ROOTIO} -DCMAKE_INSTALL_PREFIX=${HEPMC_PATH}/local -DCMAKE_INSTALL_LIBDIR=${HEPMC_PATH}/local/lib -DHEPMC3_ENABLE_PYTHON=OFF -DHEPMC3_ENABLE_SEARCH=ON -DHEPMC3_BUILD_STATIC_LIBS=OFF -DHEPMC3_CXX_STANDARD=${HEPMC3_STD}
+ CMAKE_COMMAND ${CMAKE_COMMAND}
+ CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER} -DCMAKE_CXX_FLAGS=${HEPMC_CXX_FLAGS} -DCMAKE_EXE_LINKER_FLAGS=${CMAKE_EXE_LINKER_FLAGS} -DCMAKE_SHARED_LINKER_FLAGS=${CMAKE_SHARED_LINKER_FLAGS} -DCMAKE_MODULE_LINKER_FLAGS=${CMAKE_MODULE_LINKER_FLAGS} -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -DHEPMC3_ENABLE_ROOTIO=${HEPMC3_ROOTIO} -DCMAKE_INSTALL_PREFIX=${HEPMC_PATH}/local -DCMAKE_INSTALL_LIBDIR=${HEPMC_PATH}/local/lib -DHEPMC3_ENABLE_PYTHON=OFF -DHEPMC3_ENABLE_SEARCH=ON -DHEPMC3_BUILD_STATIC_LIBS=OFF -DHEPMC3_CXX_STANDARD=${HEPMC3_STD}
+ if(OpenMP_omp_LIBRARY)
+   get_filename_component(YODA_OPENMP_LIBDIR "${OpenMP_omp_LIBRARY}" DIRECTORY)
+   set(YODA_CONFIG_LDFLAGS "${YODA_CONFIG_LDFLAGS} -L${YODA_OPENMP_LIBDIR} -Wl,-rpath,${YODA_OPENMP_LIBDIR}")
+ endif()
- CONFIGURE_COMMAND ./configure CXX=${CMAKE_CXX_COMPILER} CXXFLAGS=${FJCONTRIB_CXX_FLAGS} --fastjet-config=${fastjet_dir}/fastjet-config --prefix=${fastjet_dir}/local
+ CONFIGURE_COMMAND ./configure CXX=${CMAKE_CXX_COMPILER} CXXFLAGS=${FJCONTRIB_CXX_FLAGS} LDFLAGS=${FJCONTRIB_LINKER_FLAGS} --fastjet-config=${fastjet_dir}/fastjet-config --prefix=${fastjet_dir}/local

Why this matters

The ExternalProject configuration here is incomplete for a macOS clang build:

  • YODA needed the OpenMP library path propagated explicitly on Darwin.
  • HepMC3 needed parent linker flags propagated into the sub-build.
  • HepMC3 also needed a compatible CMake policy floor.
  • The contrib-side fjcontrib configure step also needed the linker flags propagated.

Without these, the branch does not build cleanly on macOS even when the top-level configuration is otherwise correct.

Suggested review comment

The ExternalProject configuration here is not robust enough for the macOS clang toolchain. In local builds, YODA needed the OpenMP linker path propagated explicitly on Darwin, and HepMC3 also needed the parent linker flags and a compatible CMake policy minimum forwarded into the sub-build. The contrib-side fjcontrib configure step likewise needed the linker flags propagated. Without that, the branch does not build cleanly on macOS even when the top-level preset is otherwise correct.

4. cmake/backends.cmake

What changed locally

+ set(fastjet_patch_dir "${PROJECT_SOURCE_DIR}/Backends/patches/${name}/${ver}")
- PATCH_COMMAND ""
+ PATCH_COMMAND patch -p1 < ${fastjet_patch_dir}/patch_${name}_${ver}.dif
- set(FJCONTRIB_CXX_FLAGS "${FJ_CXX_FLAGS} -std=c++17 -I${dir}/RecursiveTools")
+ set(FJCONTRIB_CXX_FLAGS "${FJ_CXX_FLAGS} -iquote${dir}/RecursiveTools")
+ if(OpenMP_omp_LIBRARY)
+   get_filename_component(FJCONTRIB_OPENMP_LIBDIR "${OpenMP_omp_LIBRARY}" DIRECTORY)
+   set(FJCONTRIB_CXX_FLAGS "${FJCONTRIB_CXX_FLAGS} -L${FJCONTRIB_OPENMP_LIBDIR} -Wl,-rpath,${FJCONTRIB_OPENMP_LIBDIR}")
+ endif()
+ if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
+   set(FJCONTRIB_CXX_FLAGS "${FJCONTRIB_CXX_FLAGS} -Wl,-headerpad_max_install_names")
+ endif()
+ set(FJCONTRIB_LD_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${NO_FIXUP_CHAINS}")
- CONFIGURE_COMMAND ./configure CXX=${CMAKE_CXX_COMPILER} CXXFLAGS=${FJCONTRIB_CXX_FLAGS} --fastjet-config=${fastjet_dir}/fastjet-config --prefix=${fastjet_dir}/local
+ CONFIGURE_COMMAND ./configure CXX=${CMAKE_CXX_COMPILER} CXXFLAGS=${FJCONTRIB_CXX_FLAGS} LDFLAGS=${FJCONTRIB_LD_FLAGS} --fastjet-config=${fastjet_dir}/fastjet-config --prefix=${fastjet_dir}/local

Why this matters

The backend build logic here was not sufficient for the macOS clang build:

  • fastjet 3.4.0 needed a real patch step.
  • fjcontrib could not be built with the current flag handling.
  • Forcing -std=c++17 was incompatible with the bundled fastjet sources because of older code patterns.
  • The RecursiveTools include path also needed more careful handling with libc++.
  • Darwin needed additional linker handling for the shared-install step.

As written, these backend steps left the branch broken on macOS.

Suggested review comment

The backend build logic here is not sufficient for the macOS clang configuration used by this branch. In local testing, fastjet 3.4.0 required an actual patch step, and fjcontrib could not be built with the current flag handling: forcing -std=c++17 is incompatible with the bundled fastjet sources, the RecursiveTools include path needs more careful handling with libc++, and Darwin also needs the appropriate linker flags for the shared-install step. As written, this backend setup leaves the macOS build broken.

5. Backends/include/gambit/Backends/backend_types/Pythia_8_312/abstract_Logger.h

What changed locally

+ #include <map>
- virtual ::std::_Rb_tree_iterator<std::pair<const std::basic_string<char>, int>> begin() =0;
- virtual ::std::_Rb_tree_iterator<std::pair<const std::basic_string<char>, int>> end() =0;
- virtual ::std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char>, int>> begin() const =0;
- virtual ::std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char>, int>> end() const =0;
+ virtual ::std::map<std::string, int>::iterator begin() =0;
+ virtual ::std::map<std::string, int>::iterator end() =0;
+ virtual ::std::map<std::string, int>::const_iterator begin() const =0;
+ virtual ::std::map<std::string, int>::const_iterator end() const =0;

Why this matters

The wrapper interface uses libstdc++ implementation details such as std::_Rb_tree_iterator. Those names are not part of the C++ standard library API and fail under libc++ on macOS.

Using the public iterator types from the actual container fixes the portability problem.

Suggested review comment

This wrapper interface uses implementation-specific iterator types such as std::_Rb_tree_iterator, which are not portable and fail under libc++ on macOS. The wrapper should use the public iterator type from the actual container, e.g. std::map<std::string, int>::iterator, instead of depending on libstdc++ internals. As written, this blocks the backend build with the clang/libc++ toolchain.

6. Backends/include/gambit/Backends/backend_types/Pythia_8_312/wrapper_Logger_decl.h

What changed locally

+ #include <map>
- ::std::_Rb_tree_iterator<std::pair<const std::basic_string<char>, int>> begin();
- ::std::_Rb_tree_iterator<std::pair<const std::basic_string<char>, int>> end();
- ::std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char>, int>> begin() const;
- ::std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char>, int>> end() const;
+ ::std::map<std::string, int>::iterator begin();
+ ::std::map<std::string, int>::iterator end();
+ ::std::map<std::string, int>::const_iterator begin() const;
+ ::std::map<std::string, int>::const_iterator end() const;

Why this matters

The declarations repeat the same non-portable dependency on internal libstdc++ tree iterator types. That is not valid across standard library implementations and breaks the macOS clang/libc++ build.

Suggested review comment

The declaration here hard-codes std::_Rb_tree_iterator / std::_Rb_tree_const_iterator, which are libstdc++ implementation details rather than standard library API. That makes the generated backend wrapper fail on macOS with libc++. Please use the corresponding std::map<std::string, int>::iterator and const_iterator types instead.

7. Backends/include/gambit/Backends/backend_types/Pythia_8_312/wrapper_Logger_def.h

What changed locally

- inline ::std::_Rb_tree_iterator<std::pair<const std::basic_string<char>, int>> Logger::begin()
+ inline ::std::map<std::string, int>::iterator Logger::begin()

- inline ::std::_Rb_tree_iterator<std::pair<const std::basic_string<char>, int>> Logger::end()
+ inline ::std::map<std::string, int>::iterator Logger::end()

- inline ::std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char>, int>> Logger::begin() const
+ inline ::std::map<std::string, int>::const_iterator Logger::begin() const

- inline ::std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char>, int>> Logger::end() const
+ inline ::std::map<std::string, int>::const_iterator Logger::end() const

Why this matters

The method definitions repeat the same libc++ portability problem from the abstract interface and declaration. The backend wrapper must use public container iterator types consistently rather than relying on implementation-specific tree iterator types.

Suggested review comment

The method definitions here again depend on std::_Rb_tree_iterator and related internal types. That is not portable across standard library implementations and fails with the macOS clang/libc++ build. The definitions need to follow the public container iterator types used in the declarations instead of relying on libstdc++ internals.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants