Upstream FastJet algorithm from MuonCollider fork

[aloeliger]

This PR includes the FastJet algorithm, made compatible with Gaudi and EDM4HEP as originally written by @samf25.

The tests run the algorithm on the PandoraPFOs of a CLD reconstruction output produced by CLDReconstruction.py, and compare the result bit by bit against the Marlin FastJetProcessor run over the same input through k4MarlinWrapper. Two configurations are covered: anti-kt inclusive, and the Valencia setup that CLDConfig itself uses for jet clustering.

Note that this now requires fastjet-contrib at build time for the ValenciaPlugin.

@tmadlener @madbaron @samf25, FYI

BEGINRELEASENOTES

• Upstream the FastJet Gaudi algorithm from the MuonCollider fork (with quite some refactoring and some testing against its Marlin pendant).

ENDRELEASENOTES

[tmadlener]

Thanks a lot for breaking this out from #50. I think in general this is in decent shape already. I am not sure I fully get why e.g. k4GaudiPandora needs to be a dependency with this (see comments below).

I have also left a few comments on the c++ part. I know most of this is pre-existing for you, but since we are here, I think we can just as well try to improve some of the things.

[aloeliger]

@tmadlener & @andresailer I have made edits for most of the existing review comments. The result builds, and I have attempted to test it, however, I will note that the current nightly build seems to have some existing errors. Upwards of 4-5 tests fail, in places this PR does not touch, and fail with memory related errors.

run_GaudiLumiCalClusterer fails with:

ApplicationMgr        INFO Application Manager Stopped successfully
ApplicationMgr        INFO Application Manager Finalized successfully
ApplicationMgr        INFO Application Manager Terminated successfully with a user requested ScheduledStop
malloc_consolidate(): unaligned fastbin chunk detected

run_ClonesAndSplitTracksFinder fails similarly except for malloc(): unsorted double linked list corrupted

run_TruthTrackFinder and run_PandoraPlutFastJet_ttbar fails with the same failure as run_GaudiLumiCalClusterer

I don't know whether this means that any testing of this PR is somewhat inconclusive, so I am making a note of it here.

[tmadlener]

Thanks a lot for all the effort so far. This is definitely an improved version to what we started with already. Given that this will probably be one of the more commonly used algorithms, I think there are still a few things that can be done in order to make it as easy to maintain and understand as possible. In the comments below it's a bit of a mix of small fixes and nit-picks and somewhat higher level design choices.

There are also a few warnings that make the CI fail (due to -Werror) that I didn't explicitly point out, but you should be able to see them from the workflow logs.

---

Given that the tests are working on the main branch it's probably something that was introduced in this PR that makes things fail. malloc issues usually mean that we delete things that we shouldn't delete. That might be the case here due to my suggestion of making some things a smart pointer that shouldn't be one because fastjet does some internal cleanup. I wasn't aware of that, so maybe changing those back will already make things work properly again.

[aloeliger]

@tmadlener I have made the additional requested changes.

I don't think the failing changes are anything I have done here, because it fails in tests that shouldn't touch FastJet, with the exact same error. I don't know where fastbin may be present elsewhere in Key4HEP, but that is the consistent failure point.

[tmadlener]

I think you will have to run clang-format to make pre-commit happy (or alternatively make your editor call it when saving files).

The CLI option is along the lines of

clang-format --style=file -i k4Reco/FastJet/include/* k4Reco/FastJet/src/*

[aloeliger]

clang-format has also been applied over emacs preferred formatting.

[tmadlener]

clang-format has also been applied over emacs preferred formatting.

Just to point this out in case you don't know (yet). You can make emacs call clang-format on save. There are various options / packages out there that automate this. Together with projectile it should automatically pick up the correct .clang-format config file for the project and format accordingly.

[tmadlener]

I have some (almost certainly) final minor comments.

pre-commit is complaining about the fact that the new files do not have the correct license header. It's probably easiest to simply copy them over from other files.

There are also a few warnings that are still flagged by the CI. From a quick look it's mostly unused parameters in functions (most easily fixed by simply removing the variable name from the function definition) and a few sign-compares.

[aloeliger]

I've touched up the few places with type or parameter warnings as well. My build on lxplus now has no warnings

[aloeliger]

I still don't have the licensing thing though, I'll include that.

[tmadlener]

One license header is still missing in the pandoraPlusFastJet.py script. It looks like the CI failures are unrelated. I will check that and see if it needs fixing elsewhere.

[jmcarcell]

Is this porting equivalent to the Marlin processor? Can we add a test that compares the output from Marlin and here? This is already done in the tests in this repository in https://github.com/key4hep/k4Reco/blob/main/test/CMakeLists.txt (but note this will have some minor changes in #54). Code for comparing (bit by bit) edm4hep::ReconstructedParticle exists in https://github.com/key4hep/k4GaudiPandora/blob/main/test/scripts/compare-pfos.py#L199. Within the same stack fastjet is the same so if the same configuration and inputs are used we should get the same back.

[aloeliger]

Is this porting equivalent to the Marlin processor? Can we add a test that compares the output from Marlin and here? This is already done in the tests in this repository in https://github.com/key4hep/k4Reco/blob/main/test/CMakeLists.txt (but note this will have some minor changes in #54). Code for comparing (bit by bit) edm4hep::ReconstructedParticle exists in https://github.com/key4hep/k4GaudiPandora/blob/main/test/scripts/compare-pfos.py#L199. Within the same stack fastjet is the same so if the same configuration and inputs are used we should get the same back.

I'm content to write this test, however I am unfamiliar with any marlin version of the FastJet processor, I've just taken over this Gaudi version for the Muon Collider project. Is there an example of that somewhere I can use to create the test?

[samf25]

I believe this is the repo with the Marlin algorithm this is based on: https://github.com/iLCSoft/MarlinFastJet/tree/master

It should have a steering file in the test directory.

[aloeliger]

I believe this is the repo with the Marlin algorithm this is based on: https://github.com/iLCSoft/MarlinFastJet/tree/master

It should have a steering file in the test directory.

The only thing in the test directory is an XML file I am not quite sure how to turn into a test or steering file. The repostiory is also ILCSoft. Do we have a marlin version of FastJet in Key4HEP?

[samf25]

Marlin steering files are XML files. So if you run Marlin steering_file.xml it will run.

[jmcarcell]

You can look it up how it has been done for ported algorithms or an LLM may do well at converting from Marlin to Gaudi. DDPlanarDigi is a very simple example: Gaudi vs Marlin . Once the steering file is there, the process is to copy a similar setup to what we have in other tests in https://github.com/key4hep/k4Reco/blob/main/test/CMakeLists.txt. A test that runs the steering file, and another test that compares. There is already a test that runs the CLDReconstruction through Marlin, that will have the PFOsFromJets collections: https://github.com/key4hep/CLDConfig/blob/main/CLDConfig/HighLevelReco/JetClusteringOrRenaming.py#L43

[aloeliger]

Okay, there should be a functioning marlin to gaudi comparison in there. It manages to pass on lxplus.

On another note, the malloc related errors seem to have gone away in the latest nightly, so these are no longer present in any test, including the fast jet ones.

[tmadlener]

I think generally this still looks fine. The biggest question that I have at the moment is whether the files in FastJet/options as well as the python/FastJet/pandoraSettings.py should go to test?

The main reason for moving them is that we would avoid giving the impression that these are somehow blessed rather than examples that we use for testing.

The main reason speaking against that is that if we have them in test they will be less discoverable as example. Not sure which option is better.

[madbaron]

I think generally this still looks fine. The biggest question that I have at the moment is whether the files in FastJet/options as well as the python/FastJet/pandoraSettings.py should go to test?

The main reason for moving them is that we would avoid giving the impression that these are somehow blessed rather than examples that we use for testing.

The main reason speaking against that is that if we have them in test they will be less discoverable as example. Not sure which option is better.

I have moved these to test and added a note to the README. I guess this is as good as it can get for now?
There's a question of consistency across the various algorithms now, but I think that should be settled elsewhere.

[tmadlener]

Merging this later today unless there are more comments.

[madbaron]

Thanks @jmcarcell for the review!

I think everything is addressed. The one place I didn't follow the suggestion is reusing the wrapper output as the Marlin
reference: the default CLD reconstruction doesn't produce jets, it only renames PandoraPFOs to PFOsFromJets.

[jmcarcell]

Both SISCone factory lambdas allocate a plugin and hand it to JetDefinition, but neither calls jetAlgo->delete_plugin_when_unused(). That ownership transfer was present before the factory change. FastJet only deletes the plugin after this opt-in, so the normal (non-iterative) SISCone configuration leaks it. Could we call it before returning jetAlgo in both lambdas? The iterative path should remain unchanged because its unique_ptr deliberately owns the plugin.

[madbaron]

I think both lambdas do call it, unless I am missed something?

registry["SISConePlugin"] = {
fastjet::SISConePlugin* pl;
pl = new fastjet::SISConePlugin(params.at(0), params.at(1));
auto jetAlgo = std::make_uniquefastjet::JetDefinition(pl);
jetAlgo->delete_plugin_when_unused(); // <--
return jetAlgo;
};

and identically for SISConeSphericalPlugin just below. The new ValenciaPlugin entry does the same.
Please let me know if I've missed the point!

[jmcarcell]

This loses ValenciaPlugin support from the original FastJet implementation in this PR: it was previously recognised and constructed, but is absent from the parameter/mode maps and factory. Was that deliberate? To keep this a faithful port, please restore it (with the necessary dependency) or explicitly document that Valencia is intentionally unsupported.

[madbaron]

I think this was dropped in the port because the image is was based on didn't ship fjcontrib. I've now restored the support for the ValenciaPlugin.

Restored, following FastJetUtil::initJetAlgo:

• find_package(FastJet) now also requires the contrib header and library, plus fastjettools/siscone/siscone_spherical.
• ValenciaPlugin added to the parameter map (3: R, beta, gamma) and the cluster mode map (FJ_exclusive_nJets | FJ_exclusive_yCut), matching Marlin's isJetAlgo("ValenciaPlugin", 3, ...) exactly.
• Factory entry mirroring the SISCone ones, including delete_plugin_when_unused().
• The hardcoded != "SISConePlugin" and != "SISConeSphericalPlugin" test that guards the algorithm-type lookup is now a NAME_TO_ALGORITHM_MAP.contains() check, so the next plugin doesn't fall into the same trap.

There is now a second comparison test, compare_ValenciaFastJet, running Gaudi against Marlin with exactly CLDConfig's parameters. Both steering files take the algorithm and clustering configuration on the command line so one pair of files serves both tests.

[jmcarcell]

The PR description appears stale after the test cleanup: it still says the tests introduce a k4GaudiPandora dependency (and potential circular dependency), but the final CMake/test files contain no k4GaudiPandora reference. The test now uses CLD input and k4MarlinWrapper/Marlin for the reference implementation. Could the description be updated to match the final dependency and test setup? The release note itself still looks accurate.

[madbaron]

The PR description appears stale after the test cleanup: it still says the tests introduce a k4GaudiPandora dependency (and potential circular dependency), but the final CMake/test files contain no k4GaudiPandora reference. The test now uses CLD input and k4MarlinWrapper/Marlin for the reference implementation. Could the description be updated to match the final dependency and test setup? The release note itself still looks accurate.

Updated!

[tmadlener]

I think this is good to go now, apart from moving one file into the test folder that I have apparently missed to flag earlier.

Otherwise all comments seem to be addressed now.

[tmadlener]

Thanks. I will merge this at some point tomorrow unless there are more comments. The ones that have been raised so far all look addressed to me.

[tmadlener]

Merging, once CI has passed