Skip to content

[hist] raise error of when axis edges are inf/nan in fixed-width histos #20201

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

[hist] raise error of when axis edges are inf/nan in fixed-width histos #20201

wants to merge 16 commits into from
+37 −12

Conversation

@ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented Oct 26, 2025 •
edited
Loading

This is related to #20174 and the discussion in #20176

Should we completely disallow infinite bin edges, since it violates a bit the assumptions of under and overflow bins? (As well as complicates the implementation of GetBinCenter, Low and Up Edge, etc). And the workaround is quite easy, just setting 1e30 / 1-e30 as variable bin edge.

Or should we only allow inf for variable bin edges, and not for fixed binwidth since there it does not make sense?

@ferdymercury ferdymercury marked this pull request as ready for review October 26, 2025 10:39
@ferdymercury ferdymercury requested a review from pcanal as a code owner October 26, 2025 12:02
@ferdymercury ferdymercury changed the title [hist] raise error of badly sorted edges as with variable binwidth [hist] raise error of badly sorted edges as with variable binwidth, and check for inf/nan Oct 26, 2025
@ferdymercury ferdymercury requested a review from dpiparo as a code owner October 26, 2025 12:55
@ferdymercury ferdymercury marked this pull request as draft October 26, 2025 13:02
@github-actions
Copy link

Test Results

    22 files      22 suites   3d 17h 34m 20s ⏱️
 3 701 tests  3 649 ✅   1 💤    51 ❌
79 426 runs  78 071 ✅ 275 💤 1 080 ❌

For more details on these failures, see this check.

Results for commit 7a477d3.

@ferdymercury ferdymercury changed the title [hist] raise error of badly sorted edges as with variable binwidth, and check for inf/nan [hist] raise error of when axis edges are inf/nan Oct 27, 2025
@ferdymercury ferdymercury marked this pull request as ready for review October 27, 2025 07:35
@ferdymercury ferdymercury changed the title [hist] raise error of when axis edges are inf/nan [hist] raise error of when axis edges are inf/nan in fixed-width histos Oct 27, 2025
@hahnjo
Copy link
Member

hahnjo commented Oct 27, 2025

@ferdymercury can you please remove the histv7 changes from this PR? It's on my list to think about what to do with non-finite arguments, but I would prefer a "global" plan of what do with them in the "new" histograms, which may or may not be the same as the "old" ones.

Also in terms of contribution style, can you please try to at least compile-test your changes before pushing and running regression tests at least once? Every action generates emails and notifications which gets annoying if there are 10 within 2 minutes...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vepadulano vepadulano Awaiting requested review from vepadulano

@hahnjo hahnjo Awaiting requested review from hahnjo hahnjo is a code owner

@lmoneta lmoneta Awaiting requested review from lmoneta lmoneta is a code owner

@guitargeek guitargeek Awaiting requested review from guitargeek

@pcanal pcanal Awaiting requested review from pcanal

@dpiparo dpiparo Awaiting requested review from dpiparo dpiparo is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

pr:squash on merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ferdymercury @hahnjo