-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add t0theoryid for the correlated replicas method (CRM) #2177
Conversation
1fc82e1
to
afa5069
Compare
afa5069
to
bcc12a0
Compare
581f020
to
50be2fb
Compare
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.
Looks good to me. If it is possible to have the t0id = tid directly in the parse rule that would be great, as it avoid having an if-else every time one of the two should appear.
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.
All good by me, just a few comments following up from those of @scarlehoff.
It's not because of the good looking. My suggestion goes in the spirit of separating completely what uses tid and what uses t0id. For instance, right now |
I see, thanks for the clarification. I may agree, although I would state it clearly in the docstring if we do that at the parse rule level. Otherwise, a newcomer would not understand. |
Agreed, if can be done (I'm not sure, produce-parse rules are something I still struggle with) it should be clear that when the t0 theoryid is not given, the normal theory id is used. |
50be2fb
to
6cd9f0a
Compare
and '9 point'. Note that these are defined in arXiv:1906.10698. This hard codes the | ||
theories needed for each prescription to avoid user error.""" | ||
prescription. The options for the latter are defined in pointprescriptions.yaml. | ||
This hard codes the theories needed for each prescription to avoid user error.""" |
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.
Actually I didn't pick this up before, why the scale variation use the t0 id and not the normal id?
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.
Both can work since the theoryids used for the scale variations depend on what is in pointprescriptions.yaml and scalevariationstheoryids.yaml. In scalevariationstheoryids the same lists of variations are repeated many times for a different theoryid
. But whether this theoryid
is the t0id or the regular theoryid is an arbitrary choice that's up to us (and I think both options are fine), but the benefit of using the t0id, is that we don't have to copy the list for all theoryids correspdonding to the alphas variations.
Also, if you use the t0id then you'll want the covmats to be the exact same for all fits with the same t0id. So if the lists were not exactly the same then this is probably the desired behaviour.
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.
Could you add a comment in the docstr with "note that this is using the t0theoryid, which by default is equal to theoryid but can be set separately" or something like this.
Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>
This is needed for the correlated replicas method.
Specifically, in the construction of the t0 covmat (which has to be the same for all values of alphas) we not only need a t0pdfset but also a t0theoryid to be the same for all values of alphas.
If we don't define a t0theoryid it uses the regular theoryid instead, which differs for different values of alphas.