-
Notifications
You must be signed in to change notification settings - Fork 6
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
Allow categorical variables in smpdf plots #1715
Conversation
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.
np.unique
seems fine to me. At the moment I don't imagine the order matters very much so as long as np.unique
keeps things consistent I couldn't care less.
Did you mean to have a different figure for the Ws and Z?
Also, there are double parentheses due to group_label
and the title both adding them.
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.
Can this be merged?
I guess it would be good to add another smpdf test in which the categorical branch is used, maybe using one of the datasets in your report @Zaharid ? ATLAS_WZ_TOT_13TEV
for instance
sm = cm.ScalarMappable(cmap=cm.viridis, norm=norm) | ||
cmap = cm.viridis | ||
#TODO: vmin vmax should be global or by figure? | ||
vmin,vmax = min(plotting_var), max(plotting_var) |
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.
vmin,vmax = min(plotting_var), max(plotting_var) | |
vmin, vmax = min(plotting_var), max(plotting_var) |
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.
There are different figures for only W and only Z but both figures contain labels for W and Z.
There are double parentheses due to group_label and the title both adding them.
bd676ee
to
0bd9df9
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.
Was this just waiting on a review or is there another reason why this hasn't been merged?
Could you run black
and isort
?
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.
Oh right, the tests...
Fix crash in plot_smpdf when the variable in the color axis is categorical, such as for the W and Z total cross sections. Encode the categorical variables as integers and use an appropriate color map to display them. The implementation feels a bit untidy, with some variables acting at a distance, but there doesn't seem to be an easy better way considering that we'd like the color bar to be the same for all figures. Also, the fact that np.unique sorts the output may not be what we want. See numpy/numpy#8621. But don't want to make it more complicated.
Overall it makes little sense to separate 3 points into 2+1
0a3c7b5
to
88a00f8
Compare
@scarlehoff @RoyStegeman I don't know what the current test failure with the evolution is, but don't believe it matters for this. Please merge this. |
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 happy with this PR. I think the error is because our tests are not compatible with the eko version and I believe this is fixed in #1754. @scarlehoff can you confirm?
Fix crash in plot_smpdf when the variable in the color axis is categorical, such as for the W and Z total cross sections. Encode the categorical variables as integers and use an appropriate color map to display them.
The implementation feels a bit untidy, with some variables acting at a distance, but there doesn't seem to be an easy better way considering that we'd like the color bar to be the same for all figures.
Also, the fact that np.unique sorts the output may not be what we want. See numpy/numpy#8621. But don't want to make it more complicated.