Fix: Handle zero valid samples in importance_reweight to avoid ValueError crash#40
Merged
kmzzhang merged 1 commit intoAug 12, 2025
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a crash in the importance_reweight function that occurs when all log weights are NaN or Inf values. The fix adds a guard to check for zero valid samples and returns zero weights as a safe fallback instead of crashing.
- Added validation to handle empty arrays before calling
.max()operation - Returns zero weights when no valid samples are available
- Added diagnostic warning message for debugging purposes
| bad = np.isnan(log_weights) + np.isinf(log_weights) | ||
| valid_weights = log_weights[~bad] | ||
| if len(valid_weights) == 0: | ||
| print("All log weights are NaN or Inf — skipping this round!") |
There was a problem hiding this comment.
Using print() for warnings is not ideal. Consider using the logging module or warnings.warn() for better control over diagnostic output and to follow Python best practices.
Suggested change
| print("All log weights are NaN or Inf — skipping this round!") | |
| warnings.warn("All log weights are NaN or Inf — skipping this round!") |
jackieblaum
added a commit
to jackieblaum/nbi
that referenced
this pull request
Apr 10, 2026
…id weights Aligns importance_reweight with upstream PR kmzzhang#40, which silently skips a round when every weight is NaN/Inf rather than crashing. Updates the test accordingly to assert zero weights are returned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds a guard in
importance_reweightto handle cases where all log weights are NaN or Inf, which previously caused a crash at.max(). If no valid samples are found, the function now returns zero weights as a safe fallback.What this fixes
Previously, if all samples were invalid (e.g., failed simulations, out-of-support prior samples), the code would hit:
leading to:
ValueError: zero-size array to reduction operation maximum which has no identityChanges
Related Issue
Fixes #39
Notes
This is a minimal fix to avoid the crash. A future enhancement could include diagnostics when invalid samples dominate, and possibly resampling strategies.