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

Allow categorical variables in smpdf plots #1715

Merged
merged 7 commits into from
Jun 19, 2023
Merged

Allow categorical variables in smpdf plots #1715

merged 7 commits into from
Jun 19, 2023

Conversation

Zaharid
Copy link
Contributor

@Zaharid Zaharid commented Apr 17, 2023

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.

@Zaharid
Copy link
Contributor Author

Zaharid commented Apr 17, 2023

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.

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.

validphys2/src/validphys/dataplots.py Outdated Show resolved Hide resolved
validphys2/src/validphys/dataplots.py Outdated Show resolved Hide resolved
Copy link
Member

@scarlehoff scarlehoff left a 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)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
vmin,vmax = min(plotting_var), max(plotting_var)
vmin, vmax = min(plotting_var), max(plotting_var)

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.

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.

@Zaharid Zaharid force-pushed the smpdfcategorical branch 2 times, most recently from bd676ee to 0bd9df9 Compare May 26, 2023 11:15
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.

Was this just waiting on a review or is there another reason why this hasn't been merged?

Could you run black and isort?

validphys2/src/validphys/tests/conftest.py Outdated Show resolved Hide resolved
validphys2/src/validphys/tests/conftest.py Outdated Show resolved Hide resolved
validphys2/src/validphys/dataplots.py Outdated Show resolved Hide resolved
validphys2/src/validphys/dataplots.py Show resolved Hide resolved
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.

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
@Zaharid
Copy link
Contributor Author

Zaharid commented Jun 19, 2023

@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.

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.

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?

@Zaharid Zaharid merged commit dfc8892 into master Jun 19, 2023
2 of 4 checks passed
@Zaharid Zaharid deleted the smpdfcategorical branch June 19, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants