Fix sfd nan at large negative x from log(0)#347
Merged
Conversation
sfd(x) computes the Fermi-Dirac entropy -f*log(f) - (1-f)*log(1-f). For x <= -39, expm1(x) rounds to -1.0 in float64, so f -> 1.0 and 1-f -> 0.0, producing log(0) -> nan and RuntimeWarnings (divide by zero in log, invalid value in multiply). The physical entropy limit there is 0 (x*log(x) -> 0 as x -> 0). Mask the f=0/1 boundaries to 0 instead of computing the log. Interior values are unchanged. This was a real numerical bug, not just noise: test_band and other tests consumed nan-valued entropy from sfd. Add a regression test covering x in [-100, 100] asserting finiteness and the vanishing boundary. Drop the NaN-tolerating filter in test_sfd_properties since sfd no longer returns nan.
Contributor
📝 WalkthroughWalkthrough
ChangesBoundary-safe Fermi-Dirac entropy
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dptb/utils/occupy.py`:
- Around line 155-161: The occupancy entropy handling in occupy.py is zeroing
every non-interior value, which unintentionally converts NaN inputs into 0.0
instead of preserving them. Update the logic around the interior mask and
out_valid initialization so only the exact boundary cases where f_valid is 0 or
1 collapse to zero, while NaN values remain NaN through the final assignment to
out[mask_in_limit].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67e8017d-04b0-428c-8ea6-2ae0d9b714f7
📒 Files selected for processing (2)
dptb/tests/test_occupy.pydptb/utils/occupy.py
Comment on lines
+155
to
+161
| interior = (f_valid > 0.0) & (f_valid < 1.0) | ||
| out_valid = np.zeros_like(f_valid) | ||
| out_valid[interior] = ( | ||
| -f_valid[interior] * np.log(f_valid[interior]) | ||
| - f1_valid[interior] * np.log(f1_valid[interior]) | ||
| ) | ||
| out[mask_in_limit] = out_valid |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve NaN propagation instead of zeroing every non-interior value.
Line 156 seeds out_valid with zeros, so this change also turns x=np.nan into 0.0 because interior is false there. That silently hides invalid upstream states; only the exact f==0/1 boundary should collapse to zero.
Suggested fix
- interior = (f_valid > 0.0) & (f_valid < 1.0)
- out_valid = np.zeros_like(f_valid)
+ boundary = np.isfinite(f_valid) & ((f_valid == 0.0) | (f_valid == 1.0))
+ interior = np.isfinite(f_valid) & ~boundary
+ out_valid = np.full_like(f_valid, np.nan, dtype=np.float64)
+ out_valid[boundary] = 0.0
out_valid[interior] = (
-f_valid[interior] * np.log(f_valid[interior])
- f1_valid[interior] * np.log(f1_valid[interior])
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| interior = (f_valid > 0.0) & (f_valid < 1.0) | |
| out_valid = np.zeros_like(f_valid) | |
| out_valid[interior] = ( | |
| -f_valid[interior] * np.log(f_valid[interior]) | |
| - f1_valid[interior] * np.log(f1_valid[interior]) | |
| ) | |
| out[mask_in_limit] = out_valid | |
| boundary = np.isfinite(f_valid) & ((f_valid == 0.0) | (f_valid == 1.0)) | |
| interior = np.isfinite(f_valid) & ~boundary | |
| out_valid = np.full_like(f_valid, np.nan, dtype=np.float64) | |
| out_valid[boundary] = 0.0 | |
| out_valid[interior] = ( | |
| -f_valid[interior] * np.log(f_valid[interior]) | |
| - f1_valid[interior] * np.log(f1_valid[interior]) | |
| ) | |
| out[mask_in_limit] = out_valid |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dptb/utils/occupy.py` around lines 155 - 161, The occupancy entropy handling
in occupy.py is zeroing every non-interior value, which unintentionally converts
NaN inputs into 0.0 instead of preserving them. Update the logic around the
interior mask and out_valid initialization so only the exact boundary cases
where f_valid is 0 or 1 collapse to zero, while NaN values remain NaN through
the final assignment to out[mask_in_limit].
|
This advisory review plan was generated from changed file names using trusted base-branch code. DeePTB PR Review Plan / DeePTB PR 审查计划Risk / 风险等级: Medium (中) · Changed files / 变更文件: 2 Why / 风险来源
Recommended Review / 建议审查重点
Detailed risk areas
Human review focus
Local commands and hold conditionsSuggested local commands:
Hold conditions:
Advisory only. / 仅作为审查辅助。 |
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
1 participant
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
sfd(x)(Fermi-Dirac entropy,-f*log(f) - (1-f)*log(1-f)) returnednanforx <= -39. For sufficiently negativex,np.expm1(x)rounds to-1.0in float64, sof -> 1.0and1-f -> 0.0, producinglog(0) -> nanplusRuntimeWarning: divide by zero in log/invalid value in multiply. The physical entropy limit there is0(x*log(x) -> 0asx -> 0).Fix: mask the
f=0/1boundaries to0instead of computing the log. Interior values are unchanged.This was a real numerical bug, not just warning noise:
test_bandand other paths consumednan-valued entropy fromsfd. The existingtest_sfd_propertieswas silently tolerating thenan(filtering withnp.isfinitebefore asserting non-negativity), which masked the issue.Reproduction (before fix)
After fix
All entries finite; boundaries vanish to
0; interior unchanged (x=0 -> ln(2)).Scope
dptb/utils/occupy.py(sfdonly) +dptb/tests/test_occupy.py(regression test + drop the nan-tolerating filter).Tests
test_sfd_finite_at_large_negative_x: asserts finiteness onx in [-100, 100], vanishing boundaries, andx=0 -> ln(2).test_sfd_propertiesupdated to assert non-negativity directly (no nan filter).dptb/tests/test_occupy.py: 54 passed.smoke or regressionsuite: 57 passed, 0 failed (no regression).test_band.py: the twooccupy.py:150RuntimeWarnings are gone.Risk
Low. The fix only changes behavior at the
f=0/1boundaries (previouslynan, now0— the physical limit). Interior values are bit-for-bit unchanged.Merge decision
Mergeable. Independent of #346 (dependency baseline); this is a code-level fix that applies regardless of numpy version.
Summary by CodeRabbit
Bug Fixes
nanresults from rounding issues.Tests