AiDotNet icon indicating copy to clipboard operation
AiDotNet copied to clipboard

Fix issue 376 and related bugs

Open ooples opened this issue 2 months ago • 1 comments

…lculators

This commit implements comprehensive unit tests for four specialized loss function fitness calculators as requested in issue #376:

  • DiceLossFitnessCalculator: Tests for medical imaging and segmentation scenarios
  • JaccardLossFitnessCalculator: Tests for object detection using IoU metrics
  • ContrastiveLossFitnessCalculator: Tests for similarity learning (face recognition)
  • CosineSimilarityLossFitnessCalculator: Tests for document similarity and embeddings

Test Coverage Includes:

  • Perfect predictions (zero loss)
  • Worst case scenarios (maximum loss)
  • Partial overlaps and varying degrees of similarity
  • Edge cases (division by zero, empty sets, all zeros)
  • Probabilistic predictions
  • Different numeric types (float, double)
  • Real-world scenarios (medical imaging, object detection, etc.)
  • Proper null handling and exception testing

Each test file contains 20+ comprehensive test cases covering:

  • Constructor behavior with different data set types
  • Mathematical correctness of loss calculations
  • IsBetterFitness comparison logic
  • ModelEvaluationData integration
  • Float and double type support
  • Magnitude invariance (for cosine similarity)
  • Imbalanced data handling

The tests follow the existing project patterns and conventions used in other loss function tests (e.g., MeanSquaredErrorLossTests, HuberLossTests).

Resolves #376

User Story / Context

  • Reference: [US-XXX] (if applicable)
  • Base branch: merge-dev2-to-master

Summary

  • What changed and why (scoped strictly to the user story / PR intent)

Verification

  • [ ] Builds succeed (scoped to changed projects)
  • [ ] Unit tests pass locally
  • [ ] Code coverage >= 90% for touched code
  • [ ] Codecov upload succeeded (if token configured)
  • [ ] TFM verification (net46, net6.0, net8.0) passes (if packaging)
  • [ ] No unresolved Copilot comments on HEAD

Copilot Review Loop (Outcome-Based)

Record counts before/after your last push:

  • Comments on HEAD BEFORE: [N]
  • Comments on HEAD AFTER (60s): [M]
  • Final HEAD SHA: [sha]

Files Modified

  • [ ] List files changed (must align with scope)

Notes

  • Any follow-ups, caveats, or migration details

ooples avatar Nov 08 '25 21:11 ooples

[!WARNING]

Rate limit exceeded

@ooples has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 19 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 4d1842a15b39cc3013e9c2a27e9f853764c8c036 and f92a93f325e0772c2378c99b486c378d435cdd1b.

📒 Files selected for processing (3)
  • .github/workflows/pr-tests.yml (1 hunks)
  • commitlint.config.js (2 hunks)
  • src/LossFunctions/RotationPredictionLoss.cs (2 hunks)

Summary by CodeRabbit

Release Notes

  • Tests

    • Added comprehensive unit test suites for fitness calculators (ContrastiveLoss, CosineSimilarityLoss, DiceLoss, JaccardLoss) covering edge cases, multiple data types, and domain scenarios.
    • Updated fairness evaluator and transaction tests with corrected test data and expectations.
  • Bug Fixes

    • Fixed tensor handling for empty/scalar inputs in data conversion utilities.
    • Improved backward pass efficiency in neural network layers using caching and optimized computations.
    • Enhanced cropping layer to properly handle image data format conversions.
  • Chores

    • Updated CI/CD workflow to enforce net8.0 framework for test execution.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

Adds comprehensive unit tests for four loss calculators and multiple internal fixes: tensor conversion edge handling, adjusted bias broadcasting and projection in layers, NHWC↔NCHW cropping/grads, rotation output type handling, ModelStats empty-input guard, and CI test workflow tightening.

Changes

Cohort / File(s) Change Summary
Loss Function Test Suites
tests/AiDotNet.Tests/UnitTests/FitnessCalculators/ContrastiveLossFitnessCalculatorTests.cs, tests/AiDotNet.Tests/UnitTests/FitnessCalculators/CosineSimilarityLossFitnessCalculatorTests.cs, tests/AiDotNet.Tests/UnitTests/FitnessCalculators/DiceLossFitnessCalculatorTests.cs, tests/AiDotNet.Tests/UnitTests/FitnessCalculators/JaccardLossFitnessCalculatorTests.cs
New, extensive unit test classes covering constructors, many CalculateFitnessScore scenarios (positive/negative/edge cases), float/double/Tensor inputs, ModelEvaluationData dataset selection, null handling, IsHigherScoreBetter/IsBetterFitness semantics, and domain scenarios.
CI Workflow
.github/workflows/pr-validation.yml
Test step now passes --framework net8.0 to dotnet dotcover test, removed continue-on-error: true, and added clarifying comments about required tests.
Tensor Conversion Helpers
src/Helpers/ConversionsHelper.cs
Improved Tensor<T> → Matrix/Vector conversions: handle scalar/empty tensors (return empty), reshape 1D tensors to 1xN row matrices, and flatten 3D+ tensors to 2D appropriately; no public signatures changed.
Rotation Loss Output Handling
src/LossFunctions/RotationPredictionLoss.cs
Return path now converts one-hot rotation labels into class-index Vector<T> when TOutput is Vector<T>; preserved augmentation logic otherwise.
Cropping Layer (NHWC↔NCHW)
src/NeuralNetworks/Layers/CroppingLayer.cs
Forward/backward convert NHWC→NCHW, call Engine.Crop/CropBackward in NCHW, then convert results back to NHWC; gradient conversions updated accordingly.
Bias Broadcasting & Patch Projection
src/NeuralNetworks/Layers/DenseLayer.cs, src/NeuralNetworks/Layers/GatedLinearUnitLayer.cs, src/NeuralNetworks/Layers/PatchEmbeddingLayer.cs
Dense/GLU: bias adds replaced with broadcast-aware TensorBroadcastAdd; PatchEmbedding: projection rewritten to flatten [B,N,patchDim]→[B*N,patchDim], matmul, then reshape to [B,N,embedDim].
Convolutional Backward Simplification
src/NeuralNetworks/Layers/ConvolutionalLayer.cs
BackwardManual now sets delta directly via ApplyActivationDerivative(_lastOutput, outputGradient) (removes separate multiplication by outputGradient).
Highway Layer Pre-activation Caching
src/NeuralNetworks/Layers/HighwayLayer.cs
Added caches for transform/gate pre-activations; forward stores them, backward uses them for derivative calculations, and ResetState clears caches.
Model Statistics Guard
src/Statistics/ModelStats.cs
ModelStats constructor skips stats calculation for empty inputs; added private IsEmptyInput helper for empty XMatrix/Tensor detection.
Test Adjustments / Expectations
tests/AiDotNet.Tests/UnitTests/Interpretability/FairnessEvaluatorTests.cs, tests/AiDotNet.Tests/UnitTests/RetrievalAugmentedGeneration/GraphTransactionTests.cs
FairnessEvaluatorTests: test data grouping updated (different per-group predictions/actuals). GraphTransactionTests: expected WAL entry count in full-ACID test changed (7 → 4).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Focus review on:
    • New loss tests: validate numeric assertions/tolerances and that test inputs match formulas.
    • CroppingLayer NHWC↔NCHW conversions and gradient correctness.
    • Bias broadcasting changes in Dense/GLU: ensure callers/weight shapes remain compatible.
    • PatchEmbedding reshape/matmul/reshape path and any memory/layout impact.
    • ConversionsHelper handling of scalar/empty/1D tensors to prevent downstream regressions.
    • HighwayLayer pre-activation caching usage and ResetState behavior.
    • RotationPredictionLoss return-type branch for Vector<T> consumers.
    • CI workflow removal of continue-on-error: confirm test suite stability.

Possibly related PRs

  • ooples/AiDotNet#478 — Overlapping changes to neural-network layer forward/backward logic (CroppingLayer, DenseLayer, ConvolutionalLayer, PatchEmbeddingLayer, HighwayLayer).
  • (no other strong code-level matches found)

Poem

🐰

I nudged the tests with whiskers bright,
Contrast and Cosine hopped into light,
Dice and Jaccard danced in tidy rows,
Tensors shaped and biases in prose,
A happy rabbit — CI lights aglow.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes several changes beyond the linked issue #376 scope: workflow YAML modifications, helper function updates (ConversionsHelper.cs), loss function changes (RotationPredictionLoss.cs), neural network layer modifications (CroppingLayer, DenseLayer, GatedLinearUnitLayer, PatchEmbeddingLayer, ConvolutionalLayer, HighwayLayer), statistical calculations (ModelStats.cs), and test data adjustments (FairnessEvaluatorTests, GraphTransactionTests). Review and document why these out-of-scope changes are necessary. Consider isolating the loss function tests into a separate PR focused solely on #376, or update the linked issue to reflect the broader scope of changes.
Docstring Coverage ⚠️ Warning Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Fix issue 376 and related bugs' is vague and does not clearly convey that the PR adds comprehensive unit tests for specialized loss function calculators. Consider revising the title to be more specific, e.g., 'Add unit tests for specialized loss function fitness calculators' to better describe the main change.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description provides comprehensive context about the unit tests being added for the four specialized loss function calculators, covering test scenarios and patterns.
Linked Issues check ✅ Passed The PR successfully implements comprehensive unit tests for all four specialized loss function calculators (Dice, Jaccard, Contrastive, CosineSimilarity) as required by issue #376, with 20+ test cases per file covering edge cases, mathematical correctness, and integration scenarios.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 08 '25 21:11 coderabbitai[bot]