Skip to content

fix: weights should not be raised to the power n in the n-th moment calculation#3729

Merged
pfackeldey merged 5 commits intoscikit-hep:mainfrom
ikrommyd:ikrommyd/fix-moment
Dec 10, 2025
Merged

fix: weights should not be raised to the power n in the n-th moment calculation#3729
pfackeldey merged 5 commits intoscikit-hep:mainfrom
ikrommyd:ikrommyd/fix-moment

Conversation

@ikrommyd
Copy link
Copy Markdown
Collaborator

@ikrommyd ikrommyd commented Nov 13, 2025

Closes #3722

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 13, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.72%. Comparing base (b749e49) to head (5cf4ca2).
⚠️ Report is 485 commits behind head on main.

Additional details and impacted files
Files with missing lines Coverage Δ
src/awkward/operations/ak_moment.py 100.00% <ø> (+8.00%) ⬆️

... and 199 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 13, 2025

The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3729

ianna
ianna requested changes Nov 13, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@ikrommyd - thanks for jumping in. Let’s discuss this at the next meeting. Thanks

pfackeldey
pfackeldey requested changes Dec 9, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@pfackeldey pfackeldey left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @ikrommyd, this is correct. The weight is not supposed to be taken to the power of n in the moment calculation.

Can you add the original issue as a test case? @T90REAL added nicely already the manual calculation for the expected moment, I'd suggest to just copy and paste this into a test case. Apparently there wasn't any test for this until now...

Afterwards we can merge this fix 👍

@ikrommyd
Copy link
Copy Markdown
Collaborator Author

ikrommyd commented Dec 9, 2025

Done @pfackeldey! Should be good

@ikrommyd ikrommyd requested a review from pfackeldey December 9, 2025 15:34
pfackeldey
pfackeldey approved these changes Dec 10, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@pfackeldey pfackeldey left a comment

Choose a reason for hiding this comment

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

Thanks!

@pfackeldey pfackeldey merged commit 306274c into scikit-hep:main Dec 10, 2025
42 checks passed
@ikrommyd ikrommyd deleted the ikrommyd/fix-moment branch December 10, 2025 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ianna ianna ianna requested changes

@pfackeldey pfackeldey pfackeldey approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ak.moment applies weights incorrectly for n≥2 (weights exponentiated)

3 participants

@ikrommyd @ianna @pfackeldey