-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Various implementations #249
Conversation
@fgrosa, do you need a review of this PR? |
Ciao @fcatalan92, if you want to check it is always welcome of course, but actually now I was waiting to add systematic uncertainties. I put it in WIP so that we still don't merge it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fgrosa, in general, the code looks fine. I have only two points:
- we would need the same implementation for the projection of trees to avoid a feature mismatch;
- probably the code could be refactored to apply the various weighting independently from each other. Now you are considering different combinations, but in principle, we could simplify the code by applying the different weighting one after another (pt, multiplicity, ...) and by projecting the sparse only at the end.
Hi @fcatalan92 thanks! I actually agree with your both comments, I just have no time right now to implement them. If it's ok for you I open an issue to remember it, and then as soon as I have time I will do it (or the efficiency weights, we might want to do a refactory after this other PR #255). In general I would merge this PR after #255 (to avoid @strogolo to resolve conflicts), but I would not wait for the refactory of the weights since the script for the results might be useful to @zhangbiao-phy @xinyepeng for the D0) |
@fgrosa Fine for me |
This PR implements: