-
Notifications
You must be signed in to change notification settings - Fork 0
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
List of metadata for pineappl grids #64
base: main
Are you sure you want to change the base?
Conversation
It seems github has some problems with PRs and is not updating at the moment and it is Friday evening, so you are spared for now - but on Monday morning I will write a loooong list of complaints |
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.
- I'm sorry to say, but most of this is non-sense and you have to do a major revision
- clearly this PR was hasty - please only ask for review when you are confident about the content
- specifically there seems to be a major misunderstanding on what pinefarm is and does - please read before the second attempt carefully(!) the documentation https://pinefarm.readthedocs.io/en/latest/ (and you may want to also have a look to the code - it is not terribly complicated)
- during reading the whole thing, please pay special attention to https://pinefarm.readthedocs.io/en/latest/pinecards/metadata.html and https://pinefarm.readthedocs.io/en/latest/output.html
- in fact, most likely what you want to do here is to open a PR in pinecards in parallel - so please also take a detailed look there https://github.com/NNPDF/pinecards and check at least (!) >= 5 cards
- remember that this is the pinefarm documentation and not the pineappl one (and in fact people are expected to be aware of the latter - so don't tell me
pineappl write --scale
rescales the grid!) - just to prove me correct about "Don't repeat yourself" and you guys wrong you added this line 89b30a6#diff-7363bdda006467e4bff004c051d8b75d5191385afd41be94e99bfe9f2df6755eR27 which is just wrong
- there are a tons of typos here and I left them intentionally
- running pre-commit is not optional!
use one of the available interfaces (including the CLI) to do so. | ||
|
||
|
||
- **Optimize and scale:** apply the built-in optimization and rescaled by a factor if needed to go e.g. from pb to fb. |
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.
that is useless
- don't tell me what the pineappl interface does
- you may want to write "since MATRIX is not doing the optimazation yet, remember to do it here" and then you should complain with MATRIX
- you may want to write "MATRIX outputs by default in fb, so remember to rescale"
|
||
- **Optimize and scale:** apply the built-in optimization and rescaled by a factor if needed to go e.g. from pb to fb. | ||
|
||
- **Delete keys:** delete `y_label_unit`, `xlabel` and `x_label-tex` |
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.
deleting keys is wrong, we need those! you may want to overwrite ...
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.
no, these keys have to be deleted! Pineappl uses x1_label, x1_label_tex, x1_unit, y_label, y_label_tex, y_unit, so keys like 'x_label', 'x_label_tex', 'x_unit_label' and 'y_label_unit' indeed have to be deleted and new keys with correct names have to be manually set. this wrong labelling of keys is a typo in matrix.
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.
Fine, but then you realize that precisely such information should be in the docs
- value | ||
- comment | ||
* - y-unit | ||
- pb/GeV |
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.
that is useless - this information is grid dependent
- pb/GeV | ||
- | ||
* - y-label | ||
- differential x-sec |
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.
this does not follow our convention
- differential x-sec | ||
- | ||
* - lumi_id_types | ||
- pdg_mc_ids |
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.
again you want to write "since MATRIX is not doing this by default, remember to add it" and then complain with MATRIX
This PR adds some docs regarding the metadata keys that one should add to a PineAPPl grid after having runned MATRIX.
@t7phy, @achiefa please have a look and feel free to change stuff.