cms-analysis / HiggsAnalysis-CombinedLimit

CMS Higgs Combination toolkit.
https://cms-analysis.github.io/HiggsAnalysis-CombinedLimit/latest
Apache License 2.0
75 stars 380 forks source link

Move combineTool.py and routines needed to create impact plots to combine #902

Closed pkausw closed 3 months ago

pkausw commented 6 months ago

This PR moves combineTool.py and the scripts needed for different helpful scripts in the CombineHarvester into combine. In particular, this will currently enable producing impact plots with combine alone.

Tested features:

adewit commented 6 months ago

While we are at it, maybe it would be good to also merge and add the various plot1Dscan.py scripts?

I think there are three existing ones:

Ideally we would have one script to replace all of these and which is added into the list of combine executables.

I would maybe decouple this from this PR - the plot1DScan scripts are a little bit of a mess, it could take time to get into reasonable shape (OTOH if Philip has the time and wants to consolidate them now I guess I will not object :') )

kcormi commented 6 months ago

While we are at it, maybe it would be good to also merge and add the various plot1Dscan.py scripts? I think there are three existing ones:

Ideally we would have one script to replace all of these and which is added into the list of combine executables.

I would maybe decouple this from this PR - the plot1DScan scripts are a little bit of a mess, it could take time to get into reasonable shape (OTOH if Philip has the time and wants to consolidate them now I guess I will not object :') )

Okay, good thinking, I hadn't looked into them to see what level of mess they were, and was hoping it was maybe a small one. :)

We can have that as a separate PR then, which Philip may or may not take care of.

Still, maybe checking through and changing the relevant import statements so that they can run without CombineHarvester would be good for this PR, I think.

ajgilbert commented 6 months ago

Hi everyone, just checking if there's any more work to do on this PR? It could be good to merge sooner than later (and have a corresponding PR in CH to remove the same scripts). Then a new tag of both to help avoid confusion - I don't know how CMSSW will handle it if there are two scripts with the same name in different packages.

anigamova commented 6 months ago

Hi @ajgilbert, @pkausw, I think the only thing left is to update the documentation, and indeed remove the combineTool.py scripts from the CH. Then we should decide how to handle the tutorial docs:

Option 1 is to just leave it as it is because they anyway refer to the sparse checkout scripts:

bash <(curl -s https://raw.githubusercontent.com/cms-analysis/CombineHarvester/main/CombineTools/scripts/sparse-checkout-https.sh)

and these scripts point to the tags, so it should work.

Option 2 is to update all of the tutorial docs and always point to the main installation instruction as done in the unfolding tutorial for example.

I think both options are fine, and the first one requires a bit less work.

ajgilbert commented 6 months ago

Hi @anigamova: i think actually we don't even need this sparse checkout scripts anymore. They were useful at the time we had a lot of analysis-specific packages in CH too, but now there's not much difference at all between the sparse checkout and just cloning the full repo. That said, we can leave them for now, to avoid having to change the docs too much.

If I understand correctly, @pkausw went through and updated most of the tutorials already to point out that CombineHarvester is no longer needed?

anigamova commented 6 months ago

I think everything looks great, I tested the setup in a clean environment with CMSSW_14_0_0_pre1 and CombineHarvester with changes from https://github.com/cms-analysis/CombineHarvester/pull/318

pkausw commented 6 months ago

Hi @ajgilbert , I went through the docs with @kcormi last week. I mainly updated the parts of the (tutorial) docs that stated that the combineTool.py is part of the CombineHarvester repo. However, I didn't remove the references to the Harvester completely from the docs because we didn't move all of the plotting scripts initially and I wanted to ensure that the instructions still work. Now that all plotting routines are moved from the Harvester, we might want to think about removing the instructions to clone the CombineHarvester repo from the tutorials altogether, but I think that's nominally not part of this PR but rather PR #908 , right?

ajgilbert commented 6 months ago

Yes, I saw #908. Ok, if all the plotting is moved then I agree, remove the instructions for CH. Thanks a lot for the effort on this!

anigamova commented 6 months ago

I think we can merge this if no one objects, but it would be good to update the #908 and remove the CombineHarvester setup instructions from the tutorials for the upcoming release.

ajgilbert commented 5 months ago

Please note a small update was made to the LimitGrid.py module in CH: https://github.com/cms-analysis/CombineHarvester/pull/319 it should be ported here