Conversation
…flicts in CI yaml files.
ATLAS stop->ZH analysis
… without ColliderBit)
|
I did a local pass on PR #574 from an isolated checkout of The main PR-specific issue I found is in
|
Detailed Review Comments for PR #574NOTE: This comment is completed with AI assistance, basically make a summary of file changes For context, the macOS build issues I reported were reproduced locally on the following machine/configuration:
The comments below are written so they can be pasted directly into GitHub review comments. For each file, I have included:
1.
|
|
Adding a simple comment to avoid forgetting, but Anders suggested that the BOSS gb.py script's dictionary at the bottom of the file could potentially solve this Rb_Tree_iterator bug, because it takes expanded types generated by castxml, and converts them back to portable types. |
|
Yeah, and just to add a quick note before I forget this too: The reason why these strange typenames are there in the first place is that these sorts of complicated-looking expanded types are what we get out of CastXML (which does the C++ code parsing) when the original code has some container type like |
|
So making the suggested change for the variadic arguments in the Macros only works for C++20 and above.
We often force C++17 in order to build with some backends, e.g. I think rivet forces it to be C++17. So whilst I support adding C++20 as an option (and I am not looking into that), I am not sure we can force requiring C++20 or above in order to build ColliderBit given that we have other codes requiring C++17 (or sometimes even C++14) |
|
I agree that we should not require C++20 just to resolve this particular macro issue, especially given that GAMBIT/ColliderBit still needs to remain compatible with backends that effectively force C++17, and in some cases even older standards. It seems the final GPT score was 6/7. |
|
Rivet should require at least C++17. We should also be compatible with higher versions, as far as I'm aware: they can be specified via the CXXFLAGS variable, IIRC. Maybe not worth the bump to C++20 for just this issue, but you want to and Rivet is a block on that, it's not intentional and we can/will fix it! |
|
Since we haven't tried to let GAMBIT build with standards above C++17 yet, it might be that some of the forcing to C++17 can be removed in the case you are building with standards above this. I will experiment with this, but might not end up putting it in this PR. |
|
I'm not quite sure what you are proposing as a solution to the macro problem @ChrisJChang ? We need some sort of fix for the macs. Are you saying we should have an option to compile with C++20? |
|
I have pushed changes that should hopefully remove this problem entirely. I got rid of the macros that used variadic arguments, in favour of a templated function. This also removes the separate "_NOCUTS" version of the define signal region macro that was being used to fix another macro problem that was previously found. Now the signal regions can be defined with: As opposed to the previous: (where N is the number of duplicate SRs to create with the same cuts, just appending an index to the SR name) It would be good if @Pengxuan-Zhu-Phys , you could test whether this fix works for your Mac. |
Thanks a lot for making this change — the move from variadic macros to templated functions looks much cleaner, and it’s great to see the separate I’m afraid I haven’t been able to complete a full build test on my Mac yet, but the reason now seems unrelated to your macro fix. At the moment I’m blocked by backend downloads: packages such as YODA cannot be fetched from HepForge during the build. I also tried accessing/downloading them manually, but the HepForge side seemed unresponsive from my end. So right now I can’t get to the stage where I can properly test whether the compilation issue is fully resolved on macOS. Once I can get the missing backends downloaded, I’ll test this again on my Mac and report back. |
|
The changes of works well. There is another small issue: Cheers |
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):
Some tests that should be run (feel free to add to):