cms-analysis / CombineHarvester

CMSSW package for the creation, editing and analysis of combine datacards and workspaces
cms-analysis.github.io/CombineHarvester/
15 stars 180 forks source link

Accidental push to cms-analysis CH instead of to the forked version #233

Closed ArturAkh closed 4 years ago

ArturAkh commented 4 years ago

Dear all,

I've accidentally pushed some fork specific changes for CH to the official CH repo without making a proper pull request: https://github.com/cms-analysis/CombineHarvester/compare/041188035d29b08580f5e11f94d79b529f7c640c..master

However, as you can see from the comparison, these are only some cosmetic changes to the (plotting), and extensions of the CMSHistFuncFactory, which are needed for MSSM application.

If you are OK with these changes, we could leave them as they are, overwise a git revert would be needed.

I apologize for this inconvenience,

Cheers,

Artur

ajgilbert commented 4 years ago

Hi Artur, the changes are probably fine but it seems like a significant amount of new code in plotImpacts.py, would like to understand it a bit better first before having it in the master branch. Maybe it's best to push a revert commit now and have a fresh PR?

cheers, Andrew

greyxray commented 4 years ago

Dear Andrew, in case there is a possibility to omit the revert I would like to elaborate a bit on it here. The default behavior of the script is actually not changed (therefore many added lines, I didn't want to touch already working code just like that). Most of the added lines are implementing an automatic page break: when the next impact in the list is by an order of magnitude smaller compared to the previous one a new page is started such that all impacts would be visible. This is not necessarily achievable in a pretty way by limiting the maximum number of nuisances per page. Cheers, Olena

ArturAkh commented 4 years ago

Hi Andrew,

Ok, it's fine with me to do a git revert now and do a proper PR to allow a possibility to comment on the changes. So if you give your "go" on this, I'll create a git revert commit.

Cheers,

Artur

ArturAkh commented 4 years ago

Ok changes reverted, PR's will follow