Skip to content
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

FTDY maxTau cuts in runcard #1733

Merged
merged 49 commits into from
May 31, 2023
Merged

FTDY maxTau cuts in runcard #1733

merged 49 commits into from
May 31, 2023

Conversation

andreab1997
Copy link
Contributor

We want to add the possibility of specifying the maxTau cut for FTDY in the runcard.

scarlehoff and others added 30 commits May 15, 2023 11:24
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
Co-authored-by: Tanjona Rabemananjara <rrabeman@nikhef.nl>
Co-authored-by: Tanjona Rabemananjara <rrabeman@nikhef.nl>
Co-authored-by: Tanjona Rabemananjara <rrabeman@nikhef.nl>
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
@andreab1997 andreab1997 requested a review from enocera May 16, 2023 09:38
@andreab1997 andreab1997 marked this pull request as ready for review May 16, 2023 10:11
@RoyStegeman RoyStegeman changed the base branch from allow_optfk to ATLAS_3D_DY_8TEV May 16, 2023 10:18
@RoyStegeman RoyStegeman changed the base branch from ATLAS_3D_DY_8TEV to allow_optfk May 16, 2023 10:18
@scarlehoff
Copy link
Member

@andreab1997 you would need to rebase this on top of #1716 once that's done to clean up the history in this PR

@RoyStegeman
Copy link
Member

The history of the pr also works with merge it just doesn't update automatically (that's why I changed the base branch). The "problem" is that some of the old commits in master are overwritten by this PR, I put problem in parentheses because it doesn't affect the diffs and I don't care that the authors of the commits change.

@scarlehoff
Copy link
Member

scarlehoff commented May 17, 2023

It's easier to review if the PR only contains the changes from the PR. Right now the diff includes changes from allow_optfk.

@RoyStegeman
Copy link
Member

Ah you're right. Do you know how github calculates those diffs? I would've thought with respect to branch we set as the base branch

@RoyStegeman RoyStegeman changed the base branch from allow_optfk to ATLAS_3D_DY_8TEV May 17, 2023 13:30
@RoyStegeman RoyStegeman changed the base branch from ATLAS_3D_DY_8TEV to allow_optfk May 17, 2023 13:30
Base automatically changed from allow_optfk to master May 19, 2023 09:09
@andreab1997
Copy link
Contributor Author

andreab1997 commented May 23, 2023

Is there anything left here? Can this be merged?

@enocera what do you think?

@RoyStegeman
Copy link
Member

The description is

We want to add the possibility of specifying the maxTau cut for FTDY in the runcard.

but then you also include min_M and max_M, though not maxY and max_rapidity. What is the reason for these choices? Also, I'm a little uncomfortable with the exception for ATLAS_DY_2D_8TEV_LOWMASS which is of course something people are not going to be aware of when they change max_M in the runcard.

P.S. if I am not mistaken @enocera wouldn't have gotten a notification for your tag if it's added in an edit

@andreab1997
Copy link
Contributor Author

The description is

We want to add the possibility of specifying the maxTau cut for FTDY in the runcard.

but then you also include min_M and max_M, though not maxY and max_rapidity. What is the reason for these choices? Also, I'm a little uncomfortable with the exception for ATLAS_DY_2D_8TEV_LOWMASS which is of course something people are not going to be aware of when they change max_M in the runcard.

P.S. if I am not mistaken @enocera wouldn't have gotten a notification for your tag if it's added in an edit

Yes you are right, we should probably choose now what we want to do. Probably the most sensible thing is to allow only the FTDY cut, do you agree?

@RoyStegeman
Copy link
Member

RoyStegeman commented May 23, 2023

I would indeed have no problem with adding the maxTau cut for FTDY in a runcard. That is assuming there is no reason to think that we'll want different values for different datasets in the future and end up having to implement an exception such as was done for ATLAS_DY_2D_8TEV_LOWMASS in this PR.

Copy link
Member

@RoyStegeman RoyStegeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this is fine, but before merging I'd indeed like @enocera to approve as well.

@scarlehoff
Copy link
Member

@enocera I'm merging this. If it shouldn't be merge please let me know and I'll revert it!

@scarlehoff scarlehoff merged commit 5b5d058 into master May 31, 2023
@scarlehoff scarlehoff deleted the FTDY_cuts branch May 31, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants