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

atlas 13tev ttb hadr implementation #1973

Merged
merged 5 commits into from
Mar 4, 2024
Merged

atlas 13tev ttb hadr implementation #1973

merged 5 commits into from
Mar 4, 2024

Conversation

t7phy
Copy link
Member

@t7phy t7phy commented Mar 1, 2024

@t7phy t7phy marked this pull request as ready for review March 3, 2024 23:43
@t7phy t7phy requested a review from scarlehoff March 3, 2024 23:43
@scarlehoff scarlehoff requested a review from enocera March 4, 2024 07:32
@scarlehoff
Copy link
Member

I think there might be some info missing or a bug somewhere in the reader since there's no label in the y axis https://vp.nnpdf.science/IsNUvaBuRLuWHFQ5CjPpVg==/#matched_datasets_from_dataspecs0_plot_fancy_dataspecs_0

(ex. of dataset with y label https://vp.nnpdf.science/oSZLrPg3Tyyf-i2mE33G-Q==/#dat_compare3_dataset_inputs0_plot_fancy_0)

Also, what does impl mean?

@t7phy t7phy changed the title atlas 13tev ttb hadr impl atlas 13tev ttb hadr implementation Mar 4, 2024
@t7phy
Copy link
Member Author

t7phy commented Mar 4, 2024

I think there might be some info missing or a bug somewhere in the reader since there's no label in the y axis https://vp.nnpdf.science/IsNUvaBuRLuWHFQ5CjPpVg==/#matched_datasets_from_dataspecs0_plot_fancy_dataspecs_0

(ex. of dataset with y label https://vp.nnpdf.science/oSZLrPg3Tyyf-i2mE33G-Q==/#dat_compare3_dataset_inputs0_plot_fancy_0)

I just checked and realized it has been missing in all reports. Possibly something to do in the reader?

Also, what does impl mean?

implementation :D

@Radonirinaunimi
Copy link
Member

Aren't you just missing the key y_label inside plotting?

@t7phy
Copy link
Member Author

t7phy commented Mar 4, 2024

Aren't you just missing the key y_label inside plotting?

I didn't even know we had a y_label in plotting block. Shouldn't the y axis label come from observable label ?

@Radonirinaunimi
Copy link
Member

Aren't you just missing the key y_label inside plotting?

I didn't even know we had a y_label in plotting block. Shouldn't the y axis label come from observable label ?

You mean dataset_label?. They don't have to be necessarily the same. dataset_label is a more generic description and appears as title in the plot while y_label exactly specifies what's on the y-axis, as you can see on the second example above.

@t7phy
Copy link
Member Author

t7phy commented Mar 4, 2024

Aren't you just missing the key y_label inside plotting?

I didn't even know we had a y_label in plotting block. Shouldn't the y axis label come from observable label ?

You mean dataset_label?. They don't have to be necessarily the same. dataset_label is a more generic description and appears as title in the plot while y_label exactly specifies what's on the y-axis, as you can see on the second example above.

No what I meant is as follows: in any of the implementation, I never added x_label or y_label in the plotting block. For x_label it always took from the label of the particular kinematic variable used, so for y_label, I assumed it would take from observable label (there is a observable key, which contains 3 more keys: description, label and units, so I am referring to that observable label). but anyway, I guess y_label will be needed in plotting block, but this will need a fix in all datasets that I implemented then.

@t7phy t7phy force-pushed the atlas_ttb_13tev_hadr branch from 2abd4a5 to a206c8d Compare March 4, 2024 15:55
@Radonirinaunimi
Copy link
Member

Yes, otherwise it'll never work.

@t7phy t7phy force-pushed the atlas_ttb_13tev_hadr branch from a206c8d to a737242 Compare March 4, 2024 15:56
@t7phy
Copy link
Member Author

t7phy commented Mar 4, 2024

Yes, otherwise it'll never work.

yes, however I think this fix would be best done in a separate PR because it needs to be done for all the datasets, and I will do that asap, so for the time being, can we atleast merge this implementation?

@Radonirinaunimi
Copy link
Member

Yes, otherwise it'll never work.

yes, however I think this fix would be best done in a separate PR because it needs to be done for all the datasets, and I will do that asap, so for the time being, can we atleast merge this implementation?

Yes, absolutely. This should be merged once fixed and @scarlehoff approves and the others will be addressed in a different PR.

Copy link
Member

@Radonirinaunimi Radonirinaunimi left a comment

Choose a reason for hiding this comment

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

Apart from the typo below, this looks good to me.

Later on, at some point, we should take some decisions regarding the utils from which more and more dataset implementations rely on.

…13TEV_HADR_DIF/metadata.yaml

Co-authored-by: Tanjona Rabemananjara <[email protected]>
@Radonirinaunimi
Copy link
Member

@t7phy, for future reference, before this PR gets merged, could you please update the report in the description? Thanks.

@t7phy
Copy link
Member Author

t7phy commented Mar 4, 2024

@t7phy, for future reference, before this PR gets merged, could you please update the report in the description? Thanks.

done! I will merge once the tests complete.

@t7phy t7phy merged commit 1623808 into master Mar 4, 2024
8 checks passed
@t7phy t7phy deleted the atlas_ttb_13tev_hadr branch March 4, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants