Repository navigation
Class zero - #99
Class zero#99
Conversation
…y adding the single bar, with an error message that displays sngle class dectected as well as an annotation and added a test for class 0 test in tests/test_bias.py
…y adding the single bar, with an error message that displays sngle class dectected as well as an annotation and added a test for class 0 test in tests/test_bias.py
…y adding the single bar, with an error message that displays sngle class dectected as well as an annotation and added a test for class 0 test in tests/test_bias.py
Khanz9664
left a comment
Khanz9664
left a comment
There was a problem hiding this comment.
Hi @jayssSmm — thank you for taking the time to work on this edge case and for revisiting the issue. The core idea of gracefully handling single-class datasets in plot_class_distribution() is valuable and aligns well with TrustLens’ stability goals.
I tested/reviewed the changes and the fallback visualization logic itself looks good. However, there are a few blocking issues that need to be addressed before we can merge this PR.
Blocking Issues
1. Corrupted GitHub Actions Workflow (.github/workflows/ci.yml)
The workflow file appears to contain merge/rebase corruption and is currently malformed.
Issues observed:
- Invalid YAML structure (e.g., misplaced
group,cancel-in-progress, incompleteschedule) - Duplicated sections (
permissions,setup-matrix,typecheck, etc.) - Invalid job structure (steps placed outside a proper
steps:block) - Duplicate commands and repeated keys
This PR is scoped to a visualization bug fix and should not modify CI infrastructure.
Action: Please revert .github/workflows/ci.yml to match main (recommended), or carefully resolve the merge/rebase issues so the file restores valid structure.
2. Unrelated Changes in ROADMAP.md
ROADMAP.md contains duplicated/accidental modifications unrelated to this issue.
Since this PR fixes a visualization edge case (Fixes #10), roadmap edits should not be included.
Action: Please revert changes to ROADMAP.md.
3. Function Signature / Docstring Regression (trustlens/visualization/bias_plots.py)
It looks like part of the original function signature and docstring may have been accidentally removed during editing.
Please restore the original public API:
def plot_class_distribution(
imbalance_data: dict,
save_path: str | None = None,
show: bool = True,
) -> plt.Figure:The return type hint, show parameter, and original docstring should remain intact for backward compatibility and documentation consistency.
Minor Cleanup
4. Duplicate Import (tests/test_bias.py)
There is a duplicate import:
equalized_odds, equalized_oddsAction: Please remove the duplicate import.
What Looks Good
- The guard clause for single-class detection is a clean solution.
- The fallback visualization prevents the Matplotlib crash gracefully.
- Returning a valid figure instead of failing preserves downstream compatibility (
TrustReport.summary_plot(), reporting utilities, etc.). - The regression test for this edge case is appreciated and moves us toward better stability.
Once the unrelated/corrupted changes are cleaned up and the original function signature is restored, I’ll be happy to review again. Thanks again for working on this!
|
Warning Review limit reached
More reviews will be available in 22 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesSingle-Class Visualization Test Coverage
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 @.github/workflows/ci.yml:
- Around line 5-6: The workflow contains a duplicate top-level "branches" key in
the workflow YAML which will break parsing; remove the duplicate key and
consolidate any branch names into a single "branches" array so there is only one
"branches" entry in the workflow definition (update the existing "branches" key
rather than adding a second one). Locate the duplicate "branches" entries in the
workflow file and merge their values into the one "branches" array, then delete
the extra "branches" key to ensure valid YAML.
- Around line 184-229: Remove the duplicate/misconfigured CI job block that
defines a second "security" job (the block that includes the artifact upload
using matrix.os and matrix.python-version) and the duplicated
"dependency-compat" job; specifically delete the security job that attempts to
upload artifacts with undefined matrix variables (references to matrix.os and
matrix.python-version) and the redundant dependency-compat job so only the
correct single security job and single dependency-compat job remain; ensure any
remaining artifact upload steps live only in jobs that actually declare the
matrix strategy.
- Around line 30-31: The CI config declares the environment variable FORCE_COLOR
twice; remove the redundant entry so only a single FORCE_COLOR: "1" remains (or
consolidate into the job-level env map if duplicates were introduced across job
steps). Update the .github/workflows CI yaml to keep one unique FORCE_COLOR key
and eliminate the duplicate to prevent YAML parsing issues and unintended
overrides.
- Around line 132-152: The job contains duplicated keys and steps—remove the
redundant entries so each item appears only once: keep a single strategy.matrix
(matrix), one checkout step (actions/checkout@v4), one setup-python step
(actions/setup-python@v5) and ensure its with: block includes python-version:
${{ matrix.python-version }} and cache: pip, keep one "Install dependencies"
step, keep a single "Run tests with coverage" step that runs pytest once, and
keep one codecov upload step with a single if: runner.os == 'Linux' condition;
update the workflow to delete the duplicated lines and consolidate the repeated
blocks (matrix, actions/checkout@v4, actions/setup-python@v5, the pytest
invocation, and the codecov if) so the job is deterministic.
- Around line 16-26: Remove the duplicated/malformed YAML block that incorrectly
nests a schedule under permissions and repeats top-level keys; specifically
delete the second "permissions:" block that includes "schedule: - cron: ..." and
the duplicated "concurrency:"/ "permissions:" entries, and instead place the
cron schedule under the proper "on: schedule:" section (use "on: schedule: -
cron: '0 0 * * 0'") and keep a single top-level "concurrency:" and single
"permissions: contents: read" entry; ensure there are no repeated top-level keys
so the workflow YAML is valid.
- Around line 71-109: Remove the duplicated CI job block: delete the entire
repeated jobs named "typecheck", "setup-matrix", and "lint" (the
incomplete/duplicate definitions for jobs typecheck, setup-matrix, and lint) so
only the original canonical job definitions remain; ensure no leftover partial
"typecheck" job (missing runs-on/steps) remains in the workflow to avoid
duplicate job keys and broken workflow parsing.
In `@ROADMAP.md`:
- Around line 99-100: Remove the duplicate Markdown header by keeping a single
"## Phase 3: Comparative & Research — *Frontier Methods*" line and deleting the
immediate repeated instance so the section header appears only once in the
document.
- Around line 3-4: Remove the duplicate metadata line by keeping only one
occurrence of the exact string "Last updated: May 2026" at the top of ROADMAP.md
and deleting the consecutive redundant line so the file contains a single "Last
updated" entry.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d788602b-77fa-44a4-9e69-b049c24bda36
📒 Files selected for processing (4)
.github/workflows/ci.ymlROADMAP.mdtests/test_bias.pytrustlens/visualization/bias_plots.py
|
i have made the necessary changes, please review it! |
Khanz9664
left a comment
Khanz9664
left a comment
There was a problem hiding this comment.
Hi @jayssSmm — thank you for the updates and for working through the earlier feedback. I re-reviewed the latest commits and the core fix for the single-class visualization still looks good. The fallback visualization and regression test are both moving in the right direction.
That said, I still need to request a few final changes before we can merge this.
The biggest concern is scope and accidental deletions. While cleaning things up, it looks like a few important pieces of code were accidentally removed.
1. Accidental Deletion of Tests (tests/test_bias.py)
In the effort to fix the duplicate import of equalized_odds, it looks like the entire TestEqualizedOdds test class (over 140 lines of code) was accidentally deleted!
Please restore the TestEqualizedOdds class and just ensure equalized_odds is imported exactly once at the top of the file.
2. Please revert .github/workflows/ci.yml to main
Even though the workflow is parsing again, this PR should not be modifying CI infrastructure. During the manual cleanup, the macOS OpenMP installation block was accidentally deleted from the tests job.
To ensure no CI logic is lost, please revert the file completely:
git checkout origin/main -- .github/workflows/ci.yml
git commit -m "revert: restore ci workflow to main"3. Please revert ROADMAP.md
Similarly, roadmap edits are unrelated to this issue (Fixes #10). Even small wording updates should go through their own PR so project planning stays clean and intentional.
Please revert:
git checkout origin/main -- ROADMAP.md
git commit -m "revert: restore roadmap to main"4. Linting is still failing
CI / Linting (pull_request) is currently red. This might actually be caused by the missing tests or imports in test_bias.py.
Could you run this locally to fix whatever is being flagged?
pre-commit run --all-files5. Quick sanity check on public API
Please double-check that plot_class_distribution() still preserves the original public signature and return type:
def plot_class_distribution(
imbalance_data: dict,
save_path: str | None = None,
show: bool = True,
) -> plt.Figure:I want to avoid accidentally introducing an API regression while fixing the edge case.
The actual visualization fix itself looks solid! Once the deleted tests are restored, the unrelated files are reverted, and linting passes, I’ll happily approve. Thanks again for sticking with this one 🚀
|
fix: restore accidentally modified files and preserve plotting API
|
There was a problem hiding this comment.
@jayssSmm Thanks for addressing the earlier feedback — I re-reviewed the latest changes and this is in much better shape now. I appreciate you taking the time to restore the deleted tests, revert the unrelated CI/ROADMAP edits, and preserve the original plotting API.
The actual single-class handling logic still looks good to me, and the regression test is exactly the kind of safeguard we want for an edge case like this.
At this point, the only thing blocking merge on my side is the failing CI checks (Linting and Security Audit). Since the core review concerns have been addressed, could you take a quick look at those failures and push fixes?
One small note for scope: since we now have the centralized visualization styling work happening in #102, let’s keep this PR narrowly focused on the single-class bug fix and avoid broader visualization refactors here. The current approach is fine for this issue.
Once CI is green, I’ll do one final pass and we should be in a good place to merge.
|
I traced the failing
The minimal patch is just import ordering plus the missing final newline: diff --git a/tests/test_bias.py b/tests/test_bias.py
@@
import numpy as np
import pytest
-from trustlens.visualization.bias_plots import plot_class_distribution
from trustlens.metrics.bias import class_imbalance_report, equalized_odds, subgroup_performance
+from trustlens.visualization.bias_plots import plot_class_distribution
@@
- assert "Single class detected" in title
\ No newline at end of file
+ assert "Single class detected" in titleVerified locally: ruff check tests/test_bias.py trustlens/visualization/bias_plots.py
ruff format --check tests/test_bias.py trustlens/visualization/bias_plots.py
git diff --check
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONPATH=. pytest -q tests/test_bias.py -k "BiasPlots or Single"The targeted tests pass: |
Reorganize imports and adjust spacing in test_bias.py.
Refactor import statements and adjust plot_class_distribution call for consistency.
i have fixed the linting errors and single class detection during plotting by myself
Commits:
@Khanz9664 Refactor imports and clean up spacing in test_bias.py 58830b9
@Khanz9664 Merge branch 'main' into class_zero ab88603
@Khanz9664 Update bias_plots.py 761fa96
@Khanz9664 Refactor imports and streamline plot_class_distribution call c8b2488
@Khanz9664 Refactor plot_class_distribution function e153ca5
@Khanz9664 Update title for single class distribution plot 2b61b3a
Khanz9664
left a comment
Khanz9664
left a comment
There was a problem hiding this comment.
Thanks for sticking with this issue and working through several rounds of review feedback.
Over the course of the review we addressed a number of unrelated issues that had found their way into the branch (workflow changes, roadmap edits, restored tests, CI cleanup, etc.), and the PR is now back to its intended scope: fixing the single-class visualization edge case in plot_class_distribution().
The core contribution here remains the same:
- graceful handling of single-class datasets
- preservation of the existing plotting API
- valid figure generation instead of failure
- regression test coverage for the edge case
I've done some final branch maintenance and CI cleanup on top of the existing work to get everything green and aligned with current main.
All checks are now passing and the implementation looks good.
Thanks again for the contribution and for helping improve the robustness of TrustLens' visualization layer.
3 participants
Pull Request Template
Summary
Fixes a visualization edge case where
plot_class_distribution()crashed when the dataset contained only a single unique class label. The plotting logic now gracefully renders a simplified single-bar chart with a warning annotation instead of raising a Matplotlib error.Apologies for the delay on this issue — it was assigned to me earlier and took me longer than expected to properly revisit, debug, and test the edge-case handling.
Related Issue
Fixes #10
Type of Change
Please check the type of change:
Changes Made
trustlens/visualization/bias_plots.pyto detect single-class datasets.matplotlib.figure.Figure.Testing
Test command used:
Pre-submit Checklist
Before submitting, please ensure you have:
pre-commit run --all-filesand all hooks passed.make test).CHANGELOG.mdif applicable.Notes for Reviewers
Previously, datasets containing only one unique class label could trigger Matplotlib errors such as:
This PR handles that edge case gracefully by rendering a safe fallback visualization while preserving compatibility with downstream reporting utilities such as
TrustReport.summary_plot().Summary by CodeRabbit