490 bug index mismatch in the plotting of fns tof#501
Merged
dodu94 merged 5 commits intoMay 19, 2026
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughA new ChangesError Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (2)
src/jade/helper/errors.py (1)
19-31: Consider storing exception attributes for programmatic access.The exception class correctly builds an informative error message. However, unlike the existing pattern where
self.messageis stored, the rawindex1,index2, andcodelibvalues are not retained as instance attributes. This could be useful if callers want to programmatically inspect the mismatched indices (e.g., for logging or recovery logic).💡 Optional: Store attributes for programmatic access
def __init__( self, index1, index2, codelib, ): + self.index1 = index1 + self.index2 = index2 + self.codelib = codelib self.message = ( f"Indices do not match between reference and {codelib}: {index1}, {index2}" ) super().__init__(self.message)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jade/helper/errors.py` around lines 19 - 31, PlotIndexMismatchError builds a message but does not retain the raw values; modify the __init__ of PlotIndexMismatchError to assign self.index1 = index1, self.index2 = index2, and self.codelib = codelib (in addition to keeping self.message and calling super().__init__(self.message)) so callers can programmatically access the mismatched indices and source.src/jade/post/plotter.py (1)
579-580: Clarify: Index assignment happens unconditionally.These lines execute regardless of whether
same_indexreturned True or False. While this handles numerical tolerance issues (which is the intended behavior per the comment on lines 546-548), it might be clearer to add a brief inline comment explaining this is intentional for handling floating-point rounding in index values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jade/post/plotter.py` around lines 579 - 580, The unconditional assignments to val2.index = val1.index and err2.index = err1.index should be explicitly documented: add a brief inline comment next to these lines (where same_index is used) stating that the assignment is intentional even when same_index returns False to normalize minor floating-point/index rounding differences and ensure consistent indexing for downstream comparisons/plotting; reference the same_index check and these four symbols (val1.index, val2.index, err1.index, err2.index) so future readers understand this tolerance-based normalization is deliberate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/jade/post/plotter.py`:
- Around line 562-573: The bug is that val1 and err1 (the reference series) are
reassigned inside the loop over self.data[1:], causing subsequent library
comparisons to use an already-filtered reference; fix this by creating local
copies (e.g., v1 = val1.copy(); e1 = err1.copy()) before applying the
missing_subcases filtering and use v1/e1 for the per-library filters (leaving
the original val1/err1 untouched) when computing val2/err2 comparisons inside
the loop that iterates self.data[1:].
- Around line 549-577: The loop inside _get_figure mutates val1/err1 in-place
when removing missing subcases, which causes earlier modifications to leak into
subsequent library comparisons; fix by making per-iteration copies of the
reference data (e.g., assign local_temp_val1 = val1.copy() and local_temp_err1 =
err1.copy() or otherwise clone the DataFrame/Series) and perform filtering on
those locals before comparing to val2/err2, ensuring val1/err1 remain unchanged
for the next iteration; also add the suggested 3-library test (lib0: a,b,c;
lib1: a,c; lib2: a,b) to validate that different missing subcases are handled
independently across iterations.
---
Nitpick comments:
In `@src/jade/helper/errors.py`:
- Around line 19-31: PlotIndexMismatchError builds a message but does not retain
the raw values; modify the __init__ of PlotIndexMismatchError to assign
self.index1 = index1, self.index2 = index2, and self.codelib = codelib (in
addition to keeping self.message and calling super().__init__(self.message)) so
callers can programmatically access the mismatched indices and source.
In `@src/jade/post/plotter.py`:
- Around line 579-580: The unconditional assignments to val2.index = val1.index
and err2.index = err1.index should be explicitly documented: add a brief inline
comment next to these lines (where same_index is used) stating that the
assignment is intentional even when same_index returns False to normalize minor
floating-point/index rounding differences and ensure consistent indexing for
downstream comparisons/plotting; reference the same_index check and these four
symbols (val1.index, val2.index, err1.index, err2.index) so future readers
understand this tolerance-based normalization is deliberate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a07099da-e1a4-4f90-98ac-734986bac859
📒 Files selected for processing (3)
src/jade/helper/errors.pysrc/jade/post/plotter.pytests/post/test_plotter.py
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/post/test_plotter.py (2)
170-174: Also assert the warning side-effect intest_missing_subcase.At Line 170-174, the test validates filtered output size, but not the warning message that this PR explicitly promises. Please capture logs (e.g.,
caplog) and assert the warning text appears.💡 Suggested test hardening
- def test_missing_subcase(self, tmpdir): + def test_missing_subcase(self, tmpdir, caplog): @@ - plot = CEPlot(cfg, data) - output = plot.plot() + plot = CEPlot(cfg, data) + with caplog.at_level("WARNING"): + output = plot.plot() # check only two rows in plot assert len(output[0][1]) == 2 + assert "will be removed from the plot" in caplog.text output[0][0].savefig(tmpdir.join("test.png"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/post/test_plotter.py` around lines 170 - 174, The test_missing_subcase currently only asserts filtered output size but not the promised warning; modify the test to capture logs (use caplog fixture) around creating and calling CEPlot(cfg, data) and assert that the expected warning message is present in caplog.records or caplog.text after calling plot.plot(); locate the assertions around CEPlot, plot.plot, and tmpdir in test_missing_subcase to add the caplog usage and the assertion that the specific warning string appears.
151-157: Optional: align fixture direction with CEPlot reference semantics.At Line 151-157, you drop subcase
"b"fromi == 1. If this test is meant to mirror production data conventions, consider dropping from the reference dataset instead (or rename libs to make intent explicit).Based on learnings: In `src/jade/post/plotter.py` `CEPlot._get_figure`, `val1`/`err1` are experimental reference data and subcase absence is expected there.🔧 Possible alignment tweak
- if result == "b" and i == 1: + if result == "b" and i == 0: continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/post/test_plotter.py` around lines 151 - 157, The test currently skips subcase "b" by checking the library index (for i in range(n_libs) ... if result == "b" and i == 1: continue); instead, align with CEPlot reference semantics by removing the subcase from the reference dataset (the experimental/reference side used to populate val1/err1 in CEPlot._get_figure) rather than from a specific lib index, or alternatively rename the lib fixtures to make the intention explicit; update the loop to conditionally skip "b" when iterating over the reference/results source used as the expected experimental data (cfg.reference or whichever fixture supplies val1/err1) so the missing subcase reflects reference-only absence, not a particular lib index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/post/test_plotter.py`:
- Around line 170-174: The test_missing_subcase currently only asserts filtered
output size but not the promised warning; modify the test to capture logs (use
caplog fixture) around creating and calling CEPlot(cfg, data) and assert that
the expected warning message is present in caplog.records or caplog.text after
calling plot.plot(); locate the assertions around CEPlot, plot.plot, and tmpdir
in test_missing_subcase to add the caplog usage and the assertion that the
specific warning string appears.
- Around line 151-157: The test currently skips subcase "b" by checking the
library index (for i in range(n_libs) ... if result == "b" and i == 1:
continue); instead, align with CEPlot reference semantics by removing the
subcase from the reference dataset (the experimental/reference side used to
populate val1/err1 in CEPlot._get_figure) rather than from a specific lib index,
or alternatively rename the lib fixtures to make the intention explicit; update
the loop to conditionally skip "b" when iterating over the reference/results
source used as the expected experimental data (cfg.reference or whichever
fixture supplies val1/err1) so the missing subcase reflects reference-only
absence, not a particular lib index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80bebc8c-5b54-4278-9606-1dbe5f5b6ae7
📒 Files selected for processing (1)
tests/post/test_plotter.py
alexvalentine94
approved these changes
May 19, 2026
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
2 participants
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.
fix #490 together with IAEA-NDS/open-benchmarks#19
The C/E plot now can accept missing subcases from one of the libraries. The missing subcase will not be plotted and a warning will be printed out.
Summary by CodeRabbit
Bug Fixes
Tests