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

fix: inverted axes for variances of 2D weighted histograms when transformed to hist #965

Merged
merged 18 commits into from
Oct 3, 2023

Conversation

ioanaif
Copy link
Collaborator

@ioanaif ioanaif commented Oct 3, 2023

No description provided.

@ioanaif ioanaif requested a review from jpivarski October 3, 2023 13:08
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

As I understand it, you're applying the same fix to _values_variances as you've applied to to_boost, and the PR will be done when that has been tested at least once locally.

@ioanaif
Copy link
Collaborator Author

ioanaif commented Oct 3, 2023

In TH2, _values_variances does perform the axis swap, thus all is good in the computation of the variances.

I removed both the previous computation and my fix to the computation from to_boost and replaced it with simply calling .variances(flow=True) since variances are already computed at that stage.

This should fix any problem with the variances' computation and remove duplication for easier debug in the future.

It's ready to go imo. ☺️

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Okay, great!

I saw that you're using self.variances, which runs self._values_variances, and so the values are being computed twice. (That's the original reason why this function reimplemented it.)

But we can arrange to have values computed only once by doing self._values_variances in the if-clause and self.values in the else-clause. In both cases, flow=True.

I think this is ready to go, but since I made an edit, I'll give you the chance to counter-edit before merging. Go ahead and merge when you think it's ready.

@ioanaif ioanaif merged commit b5e65a8 into main Oct 3, 2023
15 checks passed
@ioanaif ioanaif deleted the ioanaif/fix-inverted-axes-variances-hist-888 branch October 3, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants