feat: add BiLSTM-CRF golden example NER model (issue #898) - #899
Merged
Merged
Conversation
…hierarchy Implements the BiLSTM-CRF golden example model for issue #898 following the existing Audio/Video model patterns exactly: - NERModelVariant enum (Tiny, Small, Base, Large, XLarge) - NamedEntityRecognition added to ModelType enum - INERModel<T> interface extending IFullModel (like IVideoModel<T>) - NERNeuralNetworkBase<T> domain base (like VideoNeuralNetworkBase<T>) - SequenceLabelingNERBase<T> task base (like VideoSuperResolutionBase<T>) - BiLSTMCRFOptions options class (like BasicVSROptions) - BiLSTMCRF<T> concrete model (like BasicVSR<T>) Two constructors: ONNX inference mode and native training mode. Uses existing LSTMLayer, DenseLayer, DropoutLayer, and ConditionalRandomFieldLayer. Full serialization/deserialization support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a comprehensive NER subsystem: enums, INERModel + progress type, core NER base classes (neural/sequence/transformer/span), many concrete NER models (sequence/span/transformer families), options, LayerHelper factories, and ONNX/native dual-mode support (training + inference). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Model as "BiLSTMCRF"
participant Pre as "PreprocessTokens"
participant Layers as "BiLSTM/Projection Layers"
participant CRF as "CRF Decoder"
participant Optim as "Optimizer/Loss"
Client->>Model: TrainAsync(tokenEmbeddings, labels)
Model->>Pre: PreprocessTokens(embeddings)
Pre-->>Model: processed embeddings
Model->>Layers: Forward pass -> emission scores
Layers-->>Model: emission scores
alt UseCRF = true
Model->>CRF: Compute CRF loss & Viterbi/backprop
CRF-->>Model: loss + gradients
else UseCRF = false
Model->>Optim: Compute loss (emissions vs labels)
Optim-->>Model: loss + gradients
end
Model->>Optim: UpdateParameters(gradients)
Optim-->>Model: parameters updated
Model-->>Client: Report progress (NERTrainingProgress)
sequenceDiagram
participant Client
participant Model as "BiLSTMCRF"
participant Pre as "PreprocessTokens"
participant Mode as "IsOnnxMode"
participant Onnx as "OnnxModel"
participant Layers as "Native Layers"
participant Decoder as "CRF or Argmax"
participant Post as "PostprocessOutput"
Client->>Model: PredictLabels(tokenEmbeddings)
Model->>Pre: PreprocessTokens(embeddings)
Model->>Mode: Check IsOnnxMode
alt ONNX Mode
Mode-->>Model: true
Model->>Onnx: RunOnnxInference(input)
Onnx-->>Model: model output
else Native Mode
Mode-->>Model: false
Model->>Layers: Forward pass -> emissions
Layers-->>Model: emissions
Model->>Decoder: Decode emissions (CRF Viterbi / Argmax)
Decoder-->>Model: label indices
end
Model->>Post: PostprocessOutput(modelOutput)
Post-->>Client: label predictions
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new Named Entity Recognition (NER) “golden example” model family to AiDotNet, centered on a BiLSTM-CRF sequence-labeling model and matching the existing domain-model pattern (base class + task base + concrete model + options + enums).
Changes:
- Introduces NER model abstractions (
INERModel<T>,NERNeuralNetworkBase<T>,SequenceLabelingNERBase<T>). - Adds a concrete
BiLSTMCRF<T>model with ONNX inference + native training modes and (de)serialization support. - Extends enums with
NERModelVariantandModelType.NamedEntityRecognition.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/NER/SequenceLabeling/SequenceLabelingNERBase.cs | Adds sequence-labeling NER base with decoding helpers. |
| src/NER/SequenceLabeling/BiLSTMCRF.cs | Implements the BiLSTM-CRF concrete model + training/inference/serialization flow. |
| src/NER/Options/BiLSTMCRFOptions.cs | Adds configurable options for the BiLSTM-CRF model. |
| src/NER/NERNeuralNetworkBase.cs | Adds NER-specific neural-network base supporting ONNX/native modes. |
| src/NER/Interfaces/INERModel.cs | Defines the NER model interface and training progress reporting. |
| src/Enums/NERModelVariant.cs | Adds standard NER model size variants. |
| src/Enums/ModelType.cs | Registers NER as a first-class ModelType entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 7
🤖 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/NER/Options/BiLSTMCRFOptions.cs`:
- Around line 39-59: The copy constructor BiLSTMCRFOptions(BiLSTMCRFOptions
other) currently aliases mutable state by assigning OnnxOptions and LabelNames
by reference; modify the constructor to deep-copy these fields instead: create a
new OnnxOptions instance (via its copy constructor/Clone method or manual field
copy) and assign it to OnnxOptions, and create a new List<string> (or
appropriate collection) populated from other.LabelNames and assign it to
LabelNames so mutations on the new options do not affect the original.
In `@src/NER/SequenceLabeling/BiLSTMCRF.cs`:
- Around line 118-121: The constructor currently ignores
BiLSTMCRFOptions.LearningRate when instantiating the default optimizer; update
the BiLSTMCRF constructor so that when _optimizer is null you construct the
AdamWOptimizer<T,Tensor<T>,Tensor<T>> using the learning rate from _options
(BiLSTMCRFOptions.LearningRate) or otherwise apply that value to the optimizer
after creation; locate the code around the _options/_optimizer initialization in
the BiLSTMCRF class and ensure the AdamWOptimizer is created/configured with
_options.LearningRate rather than the hardcoded default so user config is not
silently ignored.
- Around line 252-267: The loop in Build BiLSTM-CRF currently only adds a single
forward LSTM (Layers.Add(new LSTMLayer<T>(...))) so the model is actually
unidirectional; update the loop that iterates over _options.NumLSTMLayers to add
both a forward and a backward LSTMLayer<T> per layer (use the same
inputSize/currentInputSize, hiddenDim, activations), then treat the two outputs
as concatenated so that after each BiLSTM you set currentInputSize = 2 *
hiddenDim (and set the inputShape sizes accordingly where inputShape is
constructed from currentInputSize); ensure both LSTMLayer<T> instances are added
to Layers and that any downstream code expects the doubled feature dimension.
- Around line 299-304: The Predict override in BiLSTMCRF.cs must return decoded
label predictions instead of raw network outputs: change Predict to obtain model
scores (call Forward(input) or RunOnnxInference(input) when IsOnnxMode) and then
pass those scores into the CRF decoding routine (e.g., Decode or
Viterbi/DecodeSequence method used elsewhere in this class) to produce label
IDs/tags as a Tensor<T>; apply the same change to the other Predict overload at
the 313–314 region so both ONNX and native paths perform decoding and comply
with the SequenceLabeling base contract rather than returning logits.
- Around line 93-127: The constructors for BiLSTMCRF accept external
BiLSTMCRFOptions without validation and may silently accept unsupported settings
(e.g., UseCharEmbeddings) while InitializeLayers assumes valid values; add a
private static ValidateOptions(BiLSTMCRFOptions options) method that checks
positive numeric fields (EmbeddingDimension, HiddenDimension, NumLSTMLayers,
NumLabels, MaxSequenceLength), validates DropoutRate and LearningRate ranges,
ensures LabelNames is non-null and length == NumLabels, and throws a clear
ArgumentException/ArgumentOutOfRangeException if UseCharEmbeddings is true
(since char-embeddings are not implemented). Call ValidateOptions(_options) in
both BiLSTMCRF constructors before creating OnnxModel or calling
InitializeLayers (and after assigning _options and modelPath where applicable)
so invalid configs fail fast and unsupported flags are gated.
- Around line 438-443: CreateNewInstance currently forwards the same mutable
_options reference into the new BiLSTMCRF<T> instances (in both overloads),
which can leak mutations between models; to fix it, allocate or clone a fresh
Options object and pass that copy into the BiLSTMCRF<T> constructor instead of
_options (preserve ModelPath logic around _useNativeMode and
_options.ModelPath/p), e.g., implement or call an Options.Clone/Copy constructor
and use that clonedOptions when constructing the new BiLSTMCRF<T> so each model
gets its own independent options instance.
In `@src/NER/SequenceLabeling/SequenceLabelingNERBase.cs`:
- Around line 102-127: ArgmaxDecode currently assumes emissionScores is rank-2
and builds labels of shape [seqLen], which fails for rank-3 batched inputs;
update ArgmaxDecode to branch on emissionScores.Rank (or Shape.Length): for
rank==2 keep the existing per-sequence logic, for rank==3 iterate batches and
sequences, create labels tensor of shape [batch, seqLen], compute the correct
flattened index into emissionScores.Data.Span (e.g. offset = (b*seqLen +
s)*numLabels + l) when reading scores, and write the predicted label into the
corresponding position in labels.Data.Span; ensure you return a tensor whose
shape matches the input rank (either [seqLen] or [batch, seqLen]) and reuse
NumOps.ToDouble/FromDouble for conversions as before.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/Enums/ModelType.cssrc/Enums/NERModelVariant.cssrc/NER/Interfaces/INERModel.cssrc/NER/NERNeuralNetworkBase.cssrc/NER/Options/BiLSTMCRFOptions.cssrc/NER/SequenceLabeling/BiLSTMCRF.cssrc/NER/SequenceLabeling/SequenceLabelingNERBase.cs
…m-crf - Rewrite all NER files with thorough beginner-friendly XML documentation on every public member, explaining the research paper and how it works - Update BiLSTMCRF.InitializeLayers to use LayerHelper<T>.CreateDefaultBiLSTMCRFLayers() instead of inline layer creation, matching the BasicVSR pattern - Add CreateDefaultBiLSTMCRFLayers to LayerHelper.cs with yield return pattern and research-paper-validated defaults (Lample et al., NAACL 2016) - Document every property, method, and field across INERModel, NERNeuralNetworkBase, SequenceLabelingNERBase, BiLSTMCRFOptions, and BiLSTMCRF Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (7)
src/NER/Options/BiLSTMCRFOptions.cs (1)
68-88:⚠️ Potential issue | 🟠 MajorBlocking: copy constructor still aliases mutable members.
Line 84 and Line 87 copy references (
OnnxOptions,LabelNames) instead of values, so mutating a cloned options instance can mutate the original.🔧 Suggested fix
- OnnxOptions = other.OnnxOptions; - LabelNames = other.LabelNames; + OnnxOptions = other.OnnxOptions.Clone(); // or equivalent deep-copy path in OnnxModelOptions + LabelNames = [.. other.LabelNames];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NER/Options/BiLSTMCRFOptions.cs` around lines 68 - 88, The copy constructor BiLSTMCRFOptions(BiLSTMCRFOptions other) currently aliases mutable members OnnxOptions and LabelNames which can cause mutations on a clone to affect the original; update the constructor to perform deep copies: create a new OnnxOptions instance (or call a Clone/Copy method on other.OnnxOptions) instead of assigning the reference, and copy LabelNames into a new collection/array (e.g., new list or array from other.LabelNames) so the cloned BiLSTMCRFOptions owns its own mutable state.src/NER/SequenceLabeling/BiLSTMCRF.cs (5)
289-304:⚠️ Potential issue | 🔴 CriticalBLOCKING: Constructor accepts options without validation.
The ONNX constructor validates
modelPathbut does not validateoptions. Invalid configurations (negative dimensions, mismatchedLabelNames.Length != NumLabels, unsupportedUseCharEmbeddings = true) will silently create a broken model that fails later with confusing errors.🛡️ Add validation call after options assignment
public BiLSTMCRF(NeuralNetworkArchitecture<T> architecture, string modelPath, BiLSTMCRFOptions? options = null) : base(architecture) { if (string.IsNullOrWhiteSpace(modelPath)) throw new ArgumentException("Model path cannot be null or empty.", nameof(modelPath)); _options = options ?? new BiLSTMCRFOptions(); + ValidateOptions(_options); _useNativeMode = false;Add a static validation method:
private static void ValidateOptions(BiLSTMCRFOptions options) { if (options.EmbeddingDimension <= 0) throw new ArgumentOutOfRangeException(nameof(options), "EmbeddingDimension must be positive."); if (options.HiddenDimension <= 0) throw new ArgumentOutOfRangeException(nameof(options), "HiddenDimension must be positive."); if (options.NumLSTMLayers <= 0) throw new ArgumentOutOfRangeException(nameof(options), "NumLSTMLayers must be positive."); if (options.NumLabels <= 0) throw new ArgumentOutOfRangeException(nameof(options), "NumLabels must be positive."); if (options.MaxSequenceLength <= 0) throw new ArgumentOutOfRangeException(nameof(options), "MaxSequenceLength must be positive."); if (options.DropoutRate < 0 || options.DropoutRate >= 1) throw new ArgumentOutOfRangeException(nameof(options), "DropoutRate must be in [0, 1)."); if (options.LearningRate <= 0) throw new ArgumentOutOfRangeException(nameof(options), "LearningRate must be positive."); if (options.LabelNames is null || options.LabelNames.Length != options.NumLabels) throw new ArgumentException("LabelNames length must match NumLabels.", nameof(options)); if (options.UseCharEmbeddings) throw new NotSupportedException("Character embeddings are not yet implemented in this model."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NER/SequenceLabeling/BiLSTMCRF.cs` around lines 289 - 304, The constructor BiLSTMCRF currently assigns _options without validating fields, allowing invalid configs (e.g., non‑positive dimensions, LabelNames length mismatch, unsupported UseCharEmbeddings) to create broken models; add a private static ValidateOptions(BiLSTMCRFOptions options) that enforces positive EmbeddingDimension/HiddenDimension/NumLSTMLayers/NumLabels/MaxSequenceLength, 0<=DropoutRate<1, positive LearningRate, LabelNames non‑null and LabelNames.Length == NumLabels, and throws NotSupportedException if UseCharEmbeddings is true, then call ValidateOptions(_options) immediately after _options = options ?? new BiLSTMCRFOptions() in the BiLSTMCRF constructor so invalid options are rejected before setting fields or creating the OnnxModel and calling InitializeLayers().
718-723:⚠️ Potential issue | 🟠 Major
Predictoverrides base class contract — breaks semantic consistency.The base class
SequenceLabelingNERBase.Predict(line 280-283) routes toPredictLabels, but this override returns rawForwardoutput instead. This means:
- Direct users of
Predictget raw emission scores or ONNX output- Users going through
PredictLabelsget properly postprocessed labels- This inconsistency will cause confusion and bugs
🔧 Align with base class contract
public override Tensor<T> Predict(Tensor<T> input) { - ThrowIfDisposed(); - if (IsOnnxMode) return RunOnnxInference(input); - return Forward(input); + return PredictLabels(input); }If raw forward pass is needed internally, use a private method or call
Forwarddirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NER/SequenceLabeling/BiLSTMCRF.cs` around lines 718 - 723, The override of Predict currently returns raw Forward/ONNX outputs and breaks the base class contract (SequenceLabelingNERBase.Predict) which routes through PredictLabels; update Predict to preserve ThrowIfDisposed() and the IsOnnxMode check but then call the post-processing path (i.e., invoke PredictLabels(input) or the same label-postprocessing routine the base uses) so callers receive consistent label outputs; if you still need raw emissions for internal use, expose a private method (e.g., RawForward or use Forward directly) and do not change Predict's behavior.
350-363:⚠️ Potential issue | 🟠 MajorBLOCKING: Native constructor also lacks validation, and
LearningRateoption is dead configuration.
- Same validation issues as the ONNX constructor.
- Line 356 creates
AdamWOptimizerwithout using_options.LearningRate— user configuration is silently ignored.🔧 Apply LearningRate from options
- _optimizer = optimizer ?? new AdamWOptimizer<T, Tensor<T>, Tensor<T>>(this); + _optimizer = optimizer ?? new AdamWOptimizer<T, Tensor<T>, Tensor<T>>(this, learningRate: _options.LearningRate);Note: Verify
AdamWOptimizerconstructor accepts alearningRateparameter. If not, the optimizer should expose a way to set it, or this configuration option should be removed fromBiLSTMCRFOptions.#!/bin/bash # Check AdamWOptimizer constructor signature ast-grep --pattern 'class AdamWOptimizer<$$$> { $$$ public AdamWOptimizer($$$) { $$$ } $$$ }'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NER/SequenceLabeling/BiLSTMCRF.cs` around lines 350 - 363, The BiLSTMCRF native constructor is missing the same options validation as the ONNX constructor and it ignores _options.LearningRate when creating the optimizer; update the BiLSTMCRF(NeuralNetworkArchitecture<T>..., BiLSTMCRFOptions? options..., IGradientBasedOptimizer...?) constructor to validate required option fields on _options (e.g., NumLabels > 0, EmbeddingDimension > 0, MaxSequenceLength > 0, and consistent LabelNames if used) before using them, and apply the configured learning rate when instantiating _optimizer (use AdamWOptimizer<T, Tensor<T>, Tensor<T>>(this, learningRate: _options.LearningRate) if the constructor supports it, or call the optimizer's setter/API to set the learning rate after construction; if AdamWOptimizer has no way to accept or set a learning rate, either add that API to AdamWOptimizer or remove the dead LearningRate field from BiLSTMCRFOptions and update comments accordingly.
1053-1058:⚠️ Potential issue | 🟠 Major
CreateNewInstanceshares mutable_optionsreference across instances.Passing
_optionsdirectly means mutations to the cloned model's options will affect the original and vice versa. This violates isolation expectations for cloned instances.🔧 Clone options before passing
If
BiLSTMCRFOptionshas a copy constructor orClonemethod:protected override IFullModel<T, Tensor<T>, Tensor<T>> CreateNewInstance() { if (!_useNativeMode && _options.ModelPath is { } p && !string.IsNullOrEmpty(p)) - return new BiLSTMCRF<T>(Architecture, p, _options); - return new BiLSTMCRF<T>(Architecture, _options); + return new BiLSTMCRF<T>(Architecture, p, _options.Clone()); + return new BiLSTMCRF<T>(Architecture, _options.Clone()); }Otherwise, implement a copy mechanism in
BiLSTMCRFOptions.#!/bin/bash # Check if BiLSTMCRFOptions has a Clone method or copy constructor rg -n "Clone|BiLSTMCRFOptions\(BiLSTMCRFOptions" --type cs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NER/SequenceLabeling/BiLSTMCRF.cs` around lines 1053 - 1058, CreateNewInstance is passing the mutable _options reference into new BiLSTMCRF<T> instances which shares state between clones; instead clone the options before constructing the new model (e.g. use _options.Clone() or a copy constructor like new BiLSTMCRFOptions(_options)) and pass that cloned options object into the BiLSTMCRF<T> constructor (both the branch that uses p and the branch without p); if BiLSTMCRFOptions lacks cloning support, add a Clone method or copy constructor that copies all relevant fields and use it here to ensure isolated options per instance.
757-774: 🧹 Nitpick | 🔵 Trivial
Trainmethod looks correct but couples with overriddenPredict.Line 764 calls
Predict(input)which now returns raw output (per the override), so the loss derivative computation at line 765 operates on the expected tensor shape. However, this is fragile — ifPredictis later fixed to return labels,Trainwill break.Consider calling
Forwarddirectly here for clarity:- var output = Predict(input); + var output = Forward(PreprocessTokens(input));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NER/SequenceLabeling/BiLSTMCRF.cs` around lines 757 - 774, The Train method currently calls Predict(input) which relies on Predict returning raw model outputs; to avoid fragility if Predict is later changed to return labels, call the model's Forward method directly (e.g., invoke Forward(input) to obtain the raw logits/tensor used by LossFunction.CalculateDerivative) instead of Predict, then proceed with Tensor<T>.FromVector(...), the backward loop over Layers[i].Backward(gt), and _optimizer.UpdateParameters(Layers); keep the SetTrainingMode(true/false) and the same error checks (IsOnnxMode, _optimizer) intact.src/NER/SequenceLabeling/SequenceLabelingNERBase.cs (1)
220-245:⚠️ Potential issue | 🔴 Critical
ArgmaxDecodestill only handles rank-2 tensors — batched inputs will produce incorrect output.The method assumes
emissionScoreshas shape[seqLen, numLabels]but can receive rank-3[batch, seqLen, numLabels]when processing batched inputs with CRF disabled. The indexing and output shape will be wrong.🐛 Suggested fix to handle both ranks
protected Tensor<T> ArgmaxDecode(Tensor<T> emissionScores) { - int seqLen = emissionScores.Shape[0]; - int numLabels = emissionScores.Shape[1]; - var labels = new Tensor<T>([seqLen]); - - for (int s = 0; s < seqLen; s++) + if (emissionScores.Rank == 2) { - int bestLabel = 0; - double bestScore = double.NegativeInfinity; - - for (int l = 0; l < numLabels; l++) + int seqLen = emissionScores.Shape[0]; + int numLabels = emissionScores.Shape[1]; + var labels = new Tensor<T>([seqLen]); + for (int s = 0; s < seqLen; s++) { - double score = NumOps.ToDouble(emissionScores.Data.Span[s * numLabels + l]); - if (score > bestScore) - { - bestScore = score; - bestLabel = l; - } + int bestLabel = 0; + double bestScore = double.NegativeInfinity; + for (int l = 0; l < numLabels; l++) + { + double score = NumOps.ToDouble(emissionScores.Data.Span[s * numLabels + l]); + if (score > bestScore) { bestScore = score; bestLabel = l; } + } + labels.Data.Span[s] = NumOps.FromDouble(bestLabel); } - - labels.Data.Span[s] = NumOps.FromDouble(bestLabel); + return labels; } - - return labels; + if (emissionScores.Rank == 3) + { + int batch = emissionScores.Shape[0]; + int seqLen = emissionScores.Shape[1]; + int numLabels = emissionScores.Shape[2]; + var labels = new Tensor<T>([batch, seqLen]); + for (int b = 0; b < batch; b++) + for (int s = 0; s < seqLen; s++) + { + int bestLabel = 0; + double bestScore = double.NegativeInfinity; + int baseIdx = b * seqLen * numLabels + s * numLabels; + for (int l = 0; l < numLabels; l++) + { + double score = NumOps.ToDouble(emissionScores.Data.Span[baseIdx + l]); + if (score > bestScore) { bestScore = score; bestLabel = l; } + } + labels.Data.Span[b * seqLen + s] = NumOps.FromDouble(bestLabel); + } + return labels; + } + throw new ArgumentException($"Expected rank-2 or rank-3 emission scores, got rank {emissionScores.Rank}.", nameof(emissionScores)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NER/SequenceLabeling/SequenceLabelingNERBase.cs` around lines 220 - 245, ArgmaxDecode only handles rank-2 tensors; change it to inspect emissionScores.Rank and support rank-3 batched inputs by branching: if Rank==2 keep existing behavior (shape [seqLen, numLabels]), but if Rank==3 treat Shape as [batch, seqLen, numLabels], allocate labels with shape [batch, seqLen], and iterate over batch b and timestep s computing score = NumOps.ToDouble(emissionScores.Data.Span[(b*seqLen + s)*numLabels + l]) (or equivalent index math using Shape values) to pick bestLabel, then write NumOps.FromDouble(bestLabel) into labels.Data.Span[(b*seqLen) + s]; ensure you return the labels tensor whose rank matches the input (Tensor<T> of [seqLen, numLabels] case or [batch, seqLen] case) and preserve existing variable names (ArgmaxDecode, emissionScores, labels) for easy location.
🤖 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/Helpers/LayerHelper.cs`:
- Around line 25650-25658: The DenseLayer<T> mapping (DenseLayer<T>(inputSize:
currentInputSize, outputSize: numLabels, activationFunction:
identityActivation)) assumes input vectors of size hiddenDimension but after
implementing the missing backward LSTM the BiLSTM outputs will be concatenated
(2 * hiddenDimension); update the DenseLayer inputSize to reflect that (e.g.,
set currentInputSize or the value passed to DenseLayer to 2 * hiddenDimension)
and adjust any upstream variable (currentInputSize) initialization/assignment
accordingly, and update the comment to state the input is [sequenceLength, 2 *
hiddenDimension] so the DenseLayer and documentation match the completed BiLSTM
implementation.
- Around line 25587-25593: Change the method signature of
CreateDefaultBiLSTMCRFLayers so its first parameter is
NeuralNetworkArchitecture<T> architecture (preserving the generic T) instead of
starting with embeddingDimension, and add validation that dropoutRate is within
[0.0, 1.0] (throw ArgumentOutOfRangeException with a clear message if not).
Update the method body to use the new architecture parameter as needed and
update any callers to pass the architecture first. Ensure the method still
exposes the same remaining parameters (embeddingDimension, hiddenDimension,
numLabels, numLSTMLayers, maxSequenceLength, dropoutRate) and keep behavior
otherwise unchanged.
In `@src/NER/Options/BiLSTMCRFOptions.cs`:
- Around line 31-35: The documentation in BiLSTMCRFOptions.cs claims
25-dimensional char embeddings and a 25-unit char LSTM but the actual defaults
are CharEmbeddingDimension = 30 and CharHiddenDimension = 50; update either the
doc comment to reflect the real defaults or change the default fields to match
the documented values. Locate the class BiLSTMCRFOptions and either (A) edit the
top comment to state "30-dimensional character embeddings" and "50-unit char
LSTM" to match CharEmbeddingDimension and CharHiddenDimension, or (B) change the
default assignments for CharEmbeddingDimension and CharHiddenDimension to 25 to
match the comment; ensure both the doc block and the properties
CharEmbeddingDimension and CharHiddenDimension are consistent.
- Around line 135-427: The public option properties (e.g., EmbeddingDimension,
HiddenDimension, NumLSTMLayers, NumLabels, LabelNames, MaxSequenceLength,
LearningRate, DropoutRate, UseCharEmbeddings, CharEmbeddingDimension,
CharHiddenDimension, ModelPath, OnnxOptions) currently accept invalid values;
add input validation at the options boundary by implementing a Validate method
on the BiLSTMCRFOptions class (or guarded property setters) that checks and
throws ArgumentException/InvalidOperationException for bad inputs: numeric
dimensions and counts > 0, NumLSTMLayers >= 1, MaxSequenceLength > 0, 0.0 <=
DropoutRate <= 1.0, LearningRate > 0, LabelNames non-null/non-empty and
LabelNames.Length == NumLabels (and each label non-empty), and if
UseCharEmbeddings is true enforce CharEmbeddingDimension and CharHiddenDimension
> 0; also ensure OnnxOptions is non-null and if ModelPath set validate file
extension (.onnx) if applicable; call options.Validate() early where options are
accepted (e.g., model init) so failures are fast and descriptive.
In `@src/NER/SequenceLabeling/BiLSTMCRF.cs`:
- Around line 679-697: InitializeLayers currently ignores
_options.UseCharEmbeddings causing the feature to be silently disabled; add an
explicit validation that rejects UseCharEmbeddings == true until character
embeddings are implemented by throwing/propagating a clear exception (e.g.,
NotSupportedException or ArgumentException) early in InitializeLayers (or the
existing validation method) with a message referencing that character embeddings
are not yet supported, before any use of Architecture.Layers or
LayerHelper<T>.CreateDefaultBiLSTMCRFLayers is attempted.
- Around line 503-528: TrainAsync is doing a redundant second forward pass and
computing loss incorrectly by calling Predict (which returns raw network
outputs) and leaving F1Score empty; change Train or the training loop so Train
returns or stores the computed loss (e.g., modify Train to return a double or
set a LastLoss property) and have TrainAsync use that value for
NERTrainingProgress.Loss instead of calling Predict(tokenEmbeddings), remove the
extra Predict call to avoid a double forward pass, and either compute and
populate NERTrainingProgress.F1Score by running a proper evaluation routine on a
validation set (using existing Predict->Decode pipeline) or explicitly set
F1Score to null/NaN and document it; update references to Train, TrainAsync,
Predict, LossFunction, and NERTrainingProgress accordingly.
In `@src/NER/SequenceLabeling/SequenceLabelingNERBase.cs`:
- Around line 265-277: DecodeLabels currently only decodes rank-1 tensors; add
explicit batch support by implementing a new DecodeLabelsBatch(Tensor<T>
labelIndices) that handles rank-2 [batch, seqLen] tensors (iterate over batch
dimension, decode each sequence using the same index-to-name logic with
LabelNames and default "O") and keep the existing DecodeLabels(Tensor<T>) for
single-sequence inputs; update PredictLabels to call the batch decoder when it
returns a rank-2 tensor (or the single-sequence decoder for rank-1) so callers
receive the correct decoded shape, and validate tensor rank with a clear
exception if an unsupported rank is passed.
---
Duplicate comments:
In `@src/NER/Options/BiLSTMCRFOptions.cs`:
- Around line 68-88: The copy constructor BiLSTMCRFOptions(BiLSTMCRFOptions
other) currently aliases mutable members OnnxOptions and LabelNames which can
cause mutations on a clone to affect the original; update the constructor to
perform deep copies: create a new OnnxOptions instance (or call a Clone/Copy
method on other.OnnxOptions) instead of assigning the reference, and copy
LabelNames into a new collection/array (e.g., new list or array from
other.LabelNames) so the cloned BiLSTMCRFOptions owns its own mutable state.
In `@src/NER/SequenceLabeling/BiLSTMCRF.cs`:
- Around line 289-304: The constructor BiLSTMCRF currently assigns _options
without validating fields, allowing invalid configs (e.g., non‑positive
dimensions, LabelNames length mismatch, unsupported UseCharEmbeddings) to create
broken models; add a private static ValidateOptions(BiLSTMCRFOptions options)
that enforces positive
EmbeddingDimension/HiddenDimension/NumLSTMLayers/NumLabels/MaxSequenceLength,
0<=DropoutRate<1, positive LearningRate, LabelNames non‑null and
LabelNames.Length == NumLabels, and throws NotSupportedException if
UseCharEmbeddings is true, then call ValidateOptions(_options) immediately after
_options = options ?? new BiLSTMCRFOptions() in the BiLSTMCRF constructor so
invalid options are rejected before setting fields or creating the OnnxModel and
calling InitializeLayers().
- Around line 718-723: The override of Predict currently returns raw
Forward/ONNX outputs and breaks the base class contract
(SequenceLabelingNERBase.Predict) which routes through PredictLabels; update
Predict to preserve ThrowIfDisposed() and the IsOnnxMode check but then call the
post-processing path (i.e., invoke PredictLabels(input) or the same
label-postprocessing routine the base uses) so callers receive consistent label
outputs; if you still need raw emissions for internal use, expose a private
method (e.g., RawForward or use Forward directly) and do not change Predict's
behavior.
- Around line 350-363: The BiLSTMCRF native constructor is missing the same
options validation as the ONNX constructor and it ignores _options.LearningRate
when creating the optimizer; update the
BiLSTMCRF(NeuralNetworkArchitecture<T>..., BiLSTMCRFOptions? options...,
IGradientBasedOptimizer...?) constructor to validate required option fields on
_options (e.g., NumLabels > 0, EmbeddingDimension > 0, MaxSequenceLength > 0,
and consistent LabelNames if used) before using them, and apply the configured
learning rate when instantiating _optimizer (use AdamWOptimizer<T, Tensor<T>,
Tensor<T>>(this, learningRate: _options.LearningRate) if the constructor
supports it, or call the optimizer's setter/API to set the learning rate after
construction; if AdamWOptimizer has no way to accept or set a learning rate,
either add that API to AdamWOptimizer or remove the dead LearningRate field from
BiLSTMCRFOptions and update comments accordingly.
- Around line 1053-1058: CreateNewInstance is passing the mutable _options
reference into new BiLSTMCRF<T> instances which shares state between clones;
instead clone the options before constructing the new model (e.g. use
_options.Clone() or a copy constructor like new BiLSTMCRFOptions(_options)) and
pass that cloned options object into the BiLSTMCRF<T> constructor (both the
branch that uses p and the branch without p); if BiLSTMCRFOptions lacks cloning
support, add a Clone method or copy constructor that copies all relevant fields
and use it here to ensure isolated options per instance.
- Around line 757-774: The Train method currently calls Predict(input) which
relies on Predict returning raw model outputs; to avoid fragility if Predict is
later changed to return labels, call the model's Forward method directly (e.g.,
invoke Forward(input) to obtain the raw logits/tensor used by
LossFunction.CalculateDerivative) instead of Predict, then proceed with
Tensor<T>.FromVector(...), the backward loop over Layers[i].Backward(gt), and
_optimizer.UpdateParameters(Layers); keep the SetTrainingMode(true/false) and
the same error checks (IsOnnxMode, _optimizer) intact.
In `@src/NER/SequenceLabeling/SequenceLabelingNERBase.cs`:
- Around line 220-245: ArgmaxDecode only handles rank-2 tensors; change it to
inspect emissionScores.Rank and support rank-3 batched inputs by branching: if
Rank==2 keep existing behavior (shape [seqLen, numLabels]), but if Rank==3 treat
Shape as [batch, seqLen, numLabels], allocate labels with shape [batch, seqLen],
and iterate over batch b and timestep s computing score =
NumOps.ToDouble(emissionScores.Data.Span[(b*seqLen + s)*numLabels + l]) (or
equivalent index math using Shape values) to pick bestLabel, then write
NumOps.FromDouble(bestLabel) into labels.Data.Span[(b*seqLen) + s]; ensure you
return the labels tensor whose rank matches the input (Tensor<T> of [seqLen,
numLabels] case or [batch, seqLen] case) and preserve existing variable names
(ArgmaxDecode, emissionScores, labels) for easy location.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/Helpers/LayerHelper.cssrc/NER/Interfaces/INERModel.cssrc/NER/NERNeuralNetworkBase.cssrc/NER/Options/BiLSTMCRFOptions.cssrc/NER/SequenceLabeling/BiLSTMCRF.cssrc/NER/SequenceLabeling/SequenceLabelingNERBase.cs
- Fix PostprocessOutput to always argmax-decode (CRF returns one-hot, not indices) - Fix ArgmaxDecode to handle batched 3D tensors [batch, seqLen, numLabels] - Add DecodeLabelsBatch for batched 2D label index tensors - Add sequence length padding/truncation in PreprocessTokens - Add sequence length validation in ValidateInputShape - Use BidirectionalLayer wrapping LSTM for true bidirectional processing - Implement character embedding layers via char BiLSTM in LayerHelper - Add property validation with backing fields in BiLSTMCRFOptions - Deep-copy LabelNames and OnnxOptions in copy constructor - Apply LearningRate option to AdamW optimizer constructor - Remove Predict override (base class calls PredictLabels correctly) - Use copy-constructed options in CreateNewInstance - Add options validation in BiLSTMCRF constructors - Fix TrainAsync to compute loss before weight update - Update serialization to include char embedding and learning rate fields Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
src/Helpers/LayerHelper.cs (2)
25587-25610:⚠️ Potential issue | 🔴 CriticalBLOCKING: Golden-pattern signature is still invalid, and boundary validation is incomplete.
Line 25587 must take
NeuralNetworkArchitecture<T> architectureas the first parameter. Also,maxSequenceLength,dropoutRate, and char dimensions need explicit bounds checks to fail fast on invalid config.🔧 Proposed fix
public static IEnumerable<ILayer<T>> CreateDefaultBiLSTMCRFLayers( + NeuralNetworkArchitecture<T> architecture, int embeddingDimension = 100, int hiddenDimension = 100, int numLabels = 9, int numLSTMLayers = 1, int maxSequenceLength = 256, double dropoutRate = 0.5, bool useCharEmbeddings = false, int charEmbeddingDimension = 30, int charHiddenDimension = 50) { + ArgumentNullException.ThrowIfNull(architecture); if (embeddingDimension <= 0) throw new ArgumentOutOfRangeException(nameof(embeddingDimension), $"Embedding dimension must be positive. Got: {embeddingDimension}"); @@ if (numLSTMLayers <= 0) throw new ArgumentOutOfRangeException(nameof(numLSTMLayers), $"Number of LSTM layers must be positive. Got: {numLSTMLayers}"); + if (maxSequenceLength <= 0) + throw new ArgumentOutOfRangeException(nameof(maxSequenceLength), + $"Max sequence length must be positive. Got: {maxSequenceLength}"); + if (dropoutRate < 0.0 || dropoutRate > 1.0) + throw new ArgumentOutOfRangeException(nameof(dropoutRate), + $"Dropout rate must be between 0.0 and 1.0. Got: {dropoutRate}"); + if (useCharEmbeddings) + { + if (charEmbeddingDimension <= 0) + throw new ArgumentOutOfRangeException(nameof(charEmbeddingDimension), + $"Character embedding dimension must be positive. Got: {charEmbeddingDimension}"); + if (charHiddenDimension <= 0) + throw new ArgumentOutOfRangeException(nameof(charHiddenDimension), + $"Character hidden dimension must be positive. Got: {charHiddenDimension}"); + }As per coding guidelines: “First parameter MUST be
NeuralNetworkArchitecture<T> architecture,” and production-readiness checks require complete validation for external inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helpers/LayerHelper.cs` around lines 25587 - 25610, Update the CreateDefaultBiLSTMCRFLayers method signature so its first parameter is NeuralNetworkArchitecture<T> architecture (i.e., change signature to begin with NeuralNetworkArchitecture<T> architecture, ...), and add fast-fail validation for maxSequenceLength, dropoutRate, charEmbeddingDimension, and charHiddenDimension inside the method; specifically ensure maxSequenceLength > 0, dropoutRate is within [0.0, 1.0], and charEmbeddingDimension and charHiddenDimension > 0, throwing ArgumentOutOfRangeException with the appropriate nameof(...) and clear message for each invalid parameter (keeping the existing checks for embeddingDimension, hiddenDimension, numLabels, numLSTMLayers intact) so callers fail fast on invalid configurations.
25557-25558:⚠️ Potential issue | 🟠 MajorAlign BiLSTM merge semantics with projection sizing/docs (concat vs add).
The remarks describe concatenated BiLSTM states (
2 * hiddenDimension), while the implementation comments andcurrentInputSizehandling describe element-wise add (hiddenDimension). Please make these consistent; current ambiguity risks silent dimension drift.#!/bin/bash # Verify BidirectionalLayer merge-mode semantics and output-size assumptions rg -nP --type=cs -C4 'class\s+BidirectionalLayer<|mergeMode|concatenate|concat|element-wise|outputSize' src rg -nP --type=cs -C4 'CreateDefaultBiLSTMCRFLayers|currentInputSize\s*=|DenseLayer<T>\(' src/Helpers/LayerHelper.csAs per coding guidelines, verify that layer dimensions chain correctly from one layer to the next and that documentation matches implementation.
Also applies to: 25651-25653, 25692-25698
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helpers/LayerHelper.cs` around lines 25557 - 25558, The documentation and sizing logic for the BiLSTM projection are inconsistent: comments mention concatenation (2 * hiddenDimension) while the code path for currentInputSize and downstream DenseLayer<T> assumes element-wise add (hiddenDimension). Update CreateDefaultBiLSTMCRFLayers (and any code that inspects BidirectionalLayer.mergeMode) to make the merge-mode explicit and consistent with the projection sizing: either (A) enforce/choose concatenate and set currentInputSize = 2 * hiddenDimension before the DenseLayer<T> projection and update the linear projection comment to 2 * hiddenDimension, or (B) enforce/choose element-wise add and set currentInputSize = hiddenDimension and change docs to describe element-wise addition; ensure BidirectionalLayer.mergeMode handling, any checks/readers of mergeMode, and the DenseLayer<T> input size logic are made coherent and add a verification/assertion where currentInputSize is computed to prevent silent dimension drift.src/NER/SequenceLabeling/SequenceLabelingNERBase.cs (1)
227-286:⚠️ Potential issue | 🟠 MajorGuard unsupported tensor ranks in
ArgmaxDecode.Non-2D/non-3D tensors currently fall through the 2D decoding path, which can silently decode the wrong layout. Add an explicit rank check and fail fast with a clear exception.
Proposed fix
protected Tensor<T> ArgmaxDecode(Tensor<T> emissionScores) { // Handle batched 3D input [batch, seqLen, numLabels] if (emissionScores.Rank == 3) { ... return batchLabels; } + if (emissionScores.Rank != 2) + throw new ArgumentException( + $"ArgmaxDecode expects rank-2 [seqLen, numLabels] or rank-3 [batch, seqLen, numLabels]. Got rank {emissionScores.Rank}.", + nameof(emissionScores)); + // Handle single-sequence 2D input [seqLen, numLabels] int seq = emissionScores.Shape[0]; int labels2d = emissionScores.Shape[1]; var result = new Tensor<T>([seq]); ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NER/SequenceLabeling/SequenceLabelingNERBase.cs` around lines 227 - 286, ArgmaxDecode currently assumes emissionScores is rank 2 or 3 and silently treats other ranks as 2D; update ArgmaxDecode(Tensor<T> emissionScores) to explicitly check emissionScores.Rank and throw an ArgumentException (or similar) for any rank != 2 && != 3, including a clear message that names the method (ArgmaxDecode) and the invalid rank, so callers fail fast instead of decoding the wrong layout; keep the existing 3D and 2D handling paths intact and only add the guard at the start of the method using the emissionScores.Rank value.
🤖 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/Helpers/LayerHelper.cs`:
- Around line 25539-25541: Add XML documentation for the public parameters
useCharEmbeddings, charEmbeddingDimension, and charHiddenDimension where they
are declared/used so their purpose, valid ranges, and defaults are described;
also update the LayerHelper default-creator methods (the methods that create
default LayerHelper instances) to include complete XML docblocks containing
<summary>, <param> entries for all parameters (including these three), a
<returns> description, and a <remarks> note about defaults/behavior (e.g., when
useCharEmbeddings is false the other two are ignored), referencing the symbols
useCharEmbeddings, charEmbeddingDimension, charHiddenDimension and the
LayerHelper default-creator method names so reviewers can locate the changes.
- Around line 25642-25646: The code sets currentInputSize = embeddingDimension +
charHiddenDimension when useCharEmbeddings is true but never emits the
corresponding fusion layer, so the next token BiLSTM will see incorrect shapes;
fix by inserting an explicit fusion step after the char BiLSTM (e.g., emit a
concatenation of char outputs and word embeddings and then a projection layer if
you want to reduce back to embeddingDimension), ensure the emission is placed
before the token BiLSTM is created, and only update currentInputSize to the
actual post-fusion size (embeddingDimension + charHiddenDimension for raw
concat, or embeddingDimension if you project back) after emitting the fusion
layers; reference variables/members: useCharEmbeddings, charHiddenDimension,
embeddingDimension, currentInputSize and the char BiLSTM / token BiLSTM emission
sites when making the change.
In `@src/NER/Options/BiLSTMCRFOptions.cs`:
- Line 467: The OnnxOptions property on BiLSTMCRFOptions is externally settable
but consumed as non-null later; update the property to guard against null by
replacing the auto-property with a backing field and a setter that throws
ArgumentNullException (or ArgumentException) when someone assigns null, and keep
the default initialization (new()) so existing code still gets a non-null value;
reference the BiLSTMCRFOptions class and the OnnxOptions property so the check
is applied at the options boundary where model construction expects a non-null
OnnxModelOptions.
- Around line 414-422: The LabelNames setter currently assigns the caller's
array directly (setting _labelNames = value), which allows external mutation to
affect internal state; change the setter in BiLSTMCRFOptions to validate each
label (no null/empty entries), then store a defensive copy (e.g., clone or
Array.Copy into a new string[]) instead of the original array so _labelNames
references owned immutable state; keep the existing null/empty-array check and
throw ArgumentException as before but also validate each element and clone
before assignment.
In `@src/NER/SequenceLabeling/BiLSTMCRF.cs`:
- Around line 541-543: The loss is computed against raw labels while inputs are
preprocessed/truncated/padded by PreprocessTokens to MaxSequenceLength, causing
misalignment; update the training path around
Forward(PreprocessTokens(tokenEmbeddings)) and LossFunction.CalculateLoss(...)
to apply the same truncation/padding (or masking) to labels (e.g., produce
labelsPreprocessed = PreprocessLabels(labels, MaxSequenceLength) or compute a
mask from PreprocessTokens and pass masked logits/labels into LossFunction) so
shapes and gradients match; make the same fix at the other locations where loss
is computed (the nearby calls around the second Forward/Loss invocation and at
the calls around the 780–783 region).
- Around line 855-884: PreprocessTokens currently only handles rank-2 tensors
and drops the batch dimension for batched (rank-3) inputs; update
PreprocessTokens to detect rawEmbeddings.Rank == 3, read batch =
rawEmbeddings.Shape[0], seqLen = rawEmbeddings.Shape[1], embDim =
rawEmbeddings.Shape[2], allocate padded as new Tensor<T>([batch, maxLen,
embDim]) and copy values with nested loops over b (0..batch), s
(0..min(seqLen,maxLen)), d (0..embDim) preserving batch entries; keep the
existing rank-2 path (seqLen = Shape[0] and allocate [maxLen, embDim]) and
unchanged early return for Rank < 2. Ensure you use the existing symbols
PreprocessTokens, rawEmbeddings, _options.MaxSequenceLength,
_options.EmbeddingDimension, padded and copyLen.
- Around line 533-548: In TrainAsync, fail fast when running in ONNX mode by
checking the ONNX flag at the very start of the method and throwing
NotSupportedException instead of proceeding to compute forward/loss before
Train; locate the TrainAsync method (the Task.Run block that calls
SetTrainingMode, Forward, LossFunction.CalculateLoss and then
Train(tokenEmbeddings, labels)) and add an early guard that inspects the ONNX
mode indicator used elsewhere in this class and throws NotSupportedException
immediately if ONNX is enabled.
---
Duplicate comments:
In `@src/Helpers/LayerHelper.cs`:
- Around line 25587-25610: Update the CreateDefaultBiLSTMCRFLayers method
signature so its first parameter is NeuralNetworkArchitecture<T> architecture
(i.e., change signature to begin with NeuralNetworkArchitecture<T> architecture,
...), and add fast-fail validation for maxSequenceLength, dropoutRate,
charEmbeddingDimension, and charHiddenDimension inside the method; specifically
ensure maxSequenceLength > 0, dropoutRate is within [0.0, 1.0], and
charEmbeddingDimension and charHiddenDimension > 0, throwing
ArgumentOutOfRangeException with the appropriate nameof(...) and clear message
for each invalid parameter (keeping the existing checks for embeddingDimension,
hiddenDimension, numLabels, numLSTMLayers intact) so callers fail fast on
invalid configurations.
- Around line 25557-25558: The documentation and sizing logic for the BiLSTM
projection are inconsistent: comments mention concatenation (2 *
hiddenDimension) while the code path for currentInputSize and downstream
DenseLayer<T> assumes element-wise add (hiddenDimension). Update
CreateDefaultBiLSTMCRFLayers (and any code that inspects
BidirectionalLayer.mergeMode) to make the merge-mode explicit and consistent
with the projection sizing: either (A) enforce/choose concatenate and set
currentInputSize = 2 * hiddenDimension before the DenseLayer<T> projection and
update the linear projection comment to 2 * hiddenDimension, or (B)
enforce/choose element-wise add and set currentInputSize = hiddenDimension and
change docs to describe element-wise addition; ensure
BidirectionalLayer.mergeMode handling, any checks/readers of mergeMode, and the
DenseLayer<T> input size logic are made coherent and add a
verification/assertion where currentInputSize is computed to prevent silent
dimension drift.
In `@src/NER/SequenceLabeling/SequenceLabelingNERBase.cs`:
- Around line 227-286: ArgmaxDecode currently assumes emissionScores is rank 2
or 3 and silently treats other ranks as 2D; update ArgmaxDecode(Tensor<T>
emissionScores) to explicitly check emissionScores.Rank and throw an
ArgumentException (or similar) for any rank != 2 && != 3, including a clear
message that names the method (ArgmaxDecode) and the invalid rank, so callers
fail fast instead of decoding the wrong layout; keep the existing 3D and 2D
handling paths intact and only add the guard at the start of the method using
the emissionScores.Rank value.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/Helpers/LayerHelper.cssrc/NER/Options/BiLSTMCRFOptions.cssrc/NER/SequenceLabeling/BiLSTMCRF.cssrc/NER/SequenceLabeling/SequenceLabelingNERBase.cs
Add two new sequence labeling NER models following the golden BiLSTM-CRF pattern: - CNN-BiLSTM-CRF (Ma and Hovy, ACL 2016): char-level CNN + BiLSTM + CRF with 30 filters, kernel size 3, and 200 hidden units per the original paper - LSTM-CRF (Huang et al., 2015): unidirectional LSTM + CRF for lower-latency streaming NER inference Both include validated options with copy constructors, LayerHelper methods, explicit interface implementations, and full serialization support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ra, spanbert, xlm-roberta) Add shared TransformerNERBase abstract base class and TransformerNEROptions that handle all common transformer NER logic (training, serialization, inference, metadata). Concrete models only provide model name, paper citation, and CreateNewInstance. Add CreateDefaultTransformerNERLayers to LayerHelper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 9 transformer NER models extending TransformerNERBase: - Domain-specific: BioBERT, SciBERT, FinBERT, SEC-BERT, Legal-BERT, ClinicalBERT - Lightweight: DistilBERT (6 layers), TinyBERT (4 layers, 312 hidden) - Generic: ONNX-NER for model-agnostic ONNX Runtime inference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add SpanBasedNERBase abstract base class and SpanBasedNEROptions for span-level entity extraction. Span-based models enumerate candidate entity spans and classify each, naturally handling nested entities. Add CreateDefaultSpanBasedNERLayers to LayerHelper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add three nested/overlapping NER models extending SpanBasedNERBase: - PyramidNER: hierarchical pyramid layers for nested entities - TriaffineNER: three-way span interaction (start + end + content) - W2NER: word-word relation classification for unified flat/nested NER Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 5 transformer NER models: - Few-shot/Zero-shot: TemplateNER, InstructionNER, PromptNER - Entity Linking: BLINK (bi-encoder), REL (end-to-end linker) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (5)
src/Helpers/LayerHelper.cs (5)
25642-25646:⚠️ Potential issue | 🔴 CriticalBLOCKING:
currentInputSizeis updated as if word+char fusion happened, but no fusion layer is emitted.Both flows set
currentInputSizetoembeddingDimension + charFeatureDimwithout a concatenation/projection layer in the yielded sequence, creating an input/output shape break before the next LSTM.🔧 Shape-safe baseline fix
- currentInputSize = embeddingDimension + charHiddenDimension; + // Keep aligned with actual emitted output until explicit word+char fusion is added. + currentInputSize = charHiddenDimension;- int currentInputSize = embeddingDimension + charCNNFilters; + // Keep aligned with actual emitted output until explicit word+char fusion is added. + int currentInputSize = charCNNFilters;Then add a real fusion step (concat + optional projection) before the token-level LSTM if combined features are required.
As per coding guidelines: incomplete/half-implemented code paths are blocking and dimension chaining must be correct.
Also applies to: 25789-25790
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helpers/LayerHelper.cs` around lines 25642 - 25646, The code updates currentInputSize = embeddingDimension + charHiddenDimension as if word+char fusion occurred, but never emits the fusion layer; fix by inserting a fusion operation (concatenate char features and word embeddings, then an optional projection to the expected size) into the sequence produced by LayerHelper (e.g., before the token-level LSTM that consumes currentInputSize), emit a concrete fusion layer node (concat + Linear/Projection) and update currentInputSize to the projection output size (or keep embeddingDimension if projecting back), and apply the same fix for the duplicate site where currentInputSize is set at the other occurrence (around the second reported location) so dimension chaining remains correct.
25587-25596:⚠️ Potential issue | 🔴 CriticalBLOCKING: New
CreateDefault*Layerssignatures are missing the requiredNeuralNetworkArchitecture<T> architecturefirst parameter.All new default-creator methods should follow the shared LayerHelper signature contract.
🔧 Proposed fix pattern
public static IEnumerable<ILayer<T>> CreateDefaultBiLSTMCRFLayers( + NeuralNetworkArchitecture<T> architecture, int embeddingDimension = 100, int hiddenDimension = 100, int numLabels = 9, int numLSTMLayers = 1, int maxSequenceLength = 256, double dropoutRate = 0.5, bool useCharEmbeddings = false, int charEmbeddingDimension = 30, int charHiddenDimension = 50) { + ArgumentNullException.ThrowIfNull(architecture); if (embeddingDimension <= 0) throw new ArgumentOutOfRangeException(nameof(embeddingDimension), $"Embedding dimension must be positive. Got: {embeddingDimension}");Apply the same signature pattern to
CreateDefaultCNNBiLSTMCRFLayers,CreateDefaultLSTMCRFLayers,CreateDefaultTransformerNERLayers, andCreateDefaultSpanBasedNERLayers.As per coding guidelines: "Method Signature:
public static IEnumerable<ILayer<T>> CreateDefault{ModelName}Layers(— First parameter MUST beNeuralNetworkArchitecture<T> architecture."Also applies to: 25739-25749, 25856-25863, 25942-25951, 26015-26023
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helpers/LayerHelper.cs` around lines 25587 - 25596, The CreateDefault*Layers methods (CreateDefaultBiLSTMCRFLayers, CreateDefaultCNNBiLSTMCRFLayers, CreateDefaultLSTMCRFLayers, CreateDefaultTransformerNERLayers, CreateDefaultSpanBasedNERLayers) violate the required signature contract by omitting the first parameter; change their signatures to begin with NeuralNetworkArchitecture<T> architecture as the first parameter (e.g., public static IEnumerable<ILayer<T>> CreateDefaultBiLSTMCRFLayers(NeuralNetworkArchitecture<T> architecture, int embeddingDimension = 100, ...)), and update any internal references or overloads to accept and forward this architecture parameter accordingly so the methods match the shared LayerHelper signature pattern.
25598-25610:⚠️ Potential issue | 🔴 CriticalBLOCKING: Public parameter validation is incomplete across these helpers.
Several externally supplied parameters are unchecked (
dropoutRate,maxSequenceLength,charEmbeddingDimension,charHiddenDimension,charCNNFilters,charCNNKernelSize,intermediateDimension,spanEmbeddingDimension), allowing invalid configurations to reach deeper layers.🔧 Proposed fix pattern (example on BiLSTM-CRF)
if (numLSTMLayers <= 0) throw new ArgumentOutOfRangeException(nameof(numLSTMLayers), $"Number of LSTM layers must be positive. Got: {numLSTMLayers}"); + if (maxSequenceLength <= 0) + throw new ArgumentOutOfRangeException(nameof(maxSequenceLength), + $"Maximum sequence length must be positive. Got: {maxSequenceLength}"); + if (dropoutRate < 0.0 || dropoutRate > 1.0) + throw new ArgumentOutOfRangeException(nameof(dropoutRate), + $"Dropout rate must be in [0, 1]. Got: {dropoutRate}"); + if (useCharEmbeddings) + { + if (charEmbeddingDimension <= 0) + throw new ArgumentOutOfRangeException(nameof(charEmbeddingDimension), + $"Character embedding dimension must be positive. Got: {charEmbeddingDimension}"); + if (charHiddenDimension <= 0) + throw new ArgumentOutOfRangeException(nameof(charHiddenDimension), + $"Character hidden dimension must be positive. Got: {charHiddenDimension}"); + }Mirror equivalent checks in the other
CreateDefault*Layersmethods for their method-specific parameters.As per coding guidelines: missing validation of external inputs is a blocking production-readiness issue.
Also applies to: 25750-25761, 25864-25875, 25952-25963, 26024-26035
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helpers/LayerHelper.cs` around lines 25598 - 25610, Add missing public-parameter validation to all CreateDefault*Layers helper methods (e.g., the BiLSTM-CRF variant and the other methods referenced) by checking that dropoutRate is within [0,1], and that maxSequenceLength, charEmbeddingDimension, charHiddenDimension, charCNNFilters, charCNNKernelSize, intermediateDimension, and spanEmbeddingDimension are positive (and kernel sizes/filters are integers > 0). Throw ArgumentOutOfRangeException with nameof(...) and a clear message when a check fails; mirror the existing pattern used for embeddingDimension, hiddenDimension, numLabels, and numLSTMLayers in the methods CreateDefault...Layers so each method validates its own method-specific parameters.
25530-25541:⚠️ Potential issue | 🔴 CriticalBLOCKING: XML documentation is incomplete for several public
CreateDefault*Layersmethods.Missing/required pieces include:
CreateDefaultBiLSTMCRFLayers: no<param>docs foruseCharEmbeddings,charEmbeddingDimension,charHiddenDimension.CreateDefaultSpanBasedNERLayers: missing<param>and<returns>.- CNN/LSTM/Transformer/Span methods: missing required
<para><b>For Beginners:</b>section in<remarks>.🔧 Proposed doc fixes (pattern)
/// <param name="dropoutRate">Probability of dropping activations during training (0.0 to 1.0). /// The original paper uses 0.5 dropout between LSTM layers and before the projection layer. /// Set to 0 to disable dropout entirely.</param> +/// <param name="useCharEmbeddings">Whether to enable character-level features for each token.</param> +/// <param name="charEmbeddingDimension">Character embedding dimension when character features are enabled.</param> +/// <param name="charHiddenDimension">Character BiLSTM hidden size per direction when enabled.</param>Also add full
<param>,<returns>, and<remarks><para><b>For Beginners:</b> ...blocks to eachCreateDefault*Layersmethod missing them.As per coding guidelines, LayerHelper default-creator methods require complete XML documentation with
<summary>, full<param>,<returns>, and<remarks>containing<para><b>For Beginners:</b>.Also applies to: 25594-25596, 25712-25738, 25833-25855, 25922-25941, 26005-26015
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helpers/LayerHelper.cs` around lines 25530 - 25541, The XML docs for several public factory methods are incomplete; update each CreateDefault*Layers method (including CreateDefaultBiLSTMCRFLayers, CreateDefaultSpanBasedNERLayers and the CNN/LSTM/Transformer/Span variants referenced) to include full XML documentation: add missing <param> entries for useCharEmbeddings, charEmbeddingDimension, charHiddenDimension (for CreateDefaultBiLSTMCRFLayers), add the missing <returns> element for CreateDefaultSpanBasedNERLayers, and ensure every CreateDefault*Layers method has a <remarks> section containing a <para><b>For Beginners:</b> ...</para> block plus any other required <param> tags noted in the review; keep descriptions concise and match the existing param phrasing style used elsewhere in LayerHelper to remain consistent.
25549-25559:⚠️ Potential issue | 🟠 MajorBiLSTM dimensional contract is internally inconsistent (concatenation in docs vs additive merge in implementation).
The remarks describe
2 * hiddenDimensionconcatenated BiLSTM outputs, while implementation comments and sizing use merged/additivehiddenDimension. Align one behavior end-to-end (implementation + comments + projection sizing).Use this read-only script to confirm
BidirectionalLayer<T>merge semantics and expected output size:#!/bin/bash set -euo pipefail echo "=== BidirectionalLayer<T> semantics ===" fd 'BidirectionalLayer.cs' -t f | while read -r f; do echo "---- $f ----" rg -n -C3 'class BidirectionalLayer|mergeMode|concat|concatenate|add|sum|output' "$f" done echo "=== BiLSTM helper dimension assumptions ===" rg -n -C3 'CreateDefaultBiLSTMCRFLayers|mergeMode|2 \* hiddenDimension|currentInputSize = hiddenDimension' src/Helpers/LayerHelper.csExpected result: clear evidence whether
mergeMode: trueis additive or concatenative, and then consistentcurrentInputSize/Dense input sizing should follow.
As per coding guidelines: "Check that the layer dimensions chain correctly (output of layer N = input of layer N+1)."Also applies to: 25651-25653, 25666-25672, 25690-25697
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Helpers/LayerHelper.cs` around lines 25549 - 25559, The documentation and sizing disagree about BiLSTM output shape: docs claim concatenation (2 * hiddenDimension) but the implementation uses an additive/merged output (hiddenDimension); inspect BidirectionalLayer<T> (especially the mergeMode behavior) to determine whether it concatenates or sums, then make code/comments consistent: if mergeMode implements concatenation, update CreateDefaultBiLSTMCRFLayers and any currentInputSize/projection Dense input sizing to use 2 * hiddenDimension and adjust downstream layer inputs; if mergeMode implements additive merge, update the XML comments and any references to 2 * hiddenDimension (including the projection layer description and docs in LayerHelper.cs) to hiddenDimension so layer dimensions chain correctly between BidirectionalLayer<T>, CreateDefaultBiLSTMCRFLayers, currentInputSize, and the projection Dense.
🤖 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/NER/Options/SpanBasedNEROptions.cs`:
- Around line 150-155: The DropoutRate setter in SpanBasedNEROptions currently
rejects 1.0; update its validation to match TransformerNEROptions and
CNNBiLSTMCRFOptions by allowing values in [0, 1] inclusive. Modify the
DropoutRate property setter in class SpanBasedNEROptions so it throws only when
value < 0 || value > 1 (keeping the same exception type and message), ensuring
consistent behavior across the DropoutRate validation in all NER options
classes.
- Around line 220-237: The SpanBasedNEROptions copy constructor currently
assigns OnnxOptions by reference which causes a shallow copy; change the
assignment to construct a deep copy (e.g., set OnnxOptions = new
OnnxModelOptions(other.OnnxOptions)) and mirror the pattern used in
TransformerNEROptions/CNNBiLSTMCRFOptions; also handle the case where
other.OnnxOptions may be null (assign null if so) so mutations to the copy won't
affect the original.
In `@src/NER/SequenceLabeling/CNNBiLSTMCRF.cs`:
- Around line 225-249: TrainAsync is converting large tensors to vectors via
ToVector() before calling LossFunction.CalculateLoss, which can cause needless
copies and slow hot paths; update the loss pipeline so CalculateLoss accepts
tensor/array types directly (or add an overload) and call
LossFunction.CalculateLoss(output, labels) without ToVector(), keeping
PreprocessTokens, Forward and Train usage the same; adjust the LossFunction
implementation or its interface so it can compute loss from the original tensor
types returned by Forward and make sure TrainAsync (the method containing
Forward, PreprocessTokens, ToVector, Train) removes the ToVector() calls to
avoid extra allocations.
In `@src/NER/SequenceLabeling/LSTMCRF.cs`:
- Around line 213-217: The ValidateInputShape accepts rank-3 [batch, seqLen,
embDim] but the preprocessing in LSTMCRF treats input as rank-2 and allocates
[maxLen, embDim], which will break batched inputs; update the preprocessing path
in LSTMCRF so that when Tensor<T>.Rank == 3 you handle the batch dimension
explicitly (either loop over batch slices or allocate/process a [batch, maxLen,
embDim] buffer and adjust downstream indexing), update any loops that assume a
single sequence length to iterate per batch, and ensure both preprocessing
branches (the block around the current rank-2 handling and the alternative
branch around the same area referenced in the comment) are corrected to preserve
batch shape and return/emit per-example outputs accordingly (keep
ValidateInputShape as-is but make Preprocessing/Forward in LSTMCRF consistent
with rank-3 inputs).
- Around line 170-200: The training loop in INERModel<T>.TrainAsync calls
PreprocessTokens(tokenEmbeddings) and uses Forward(...) to produce output but
still computes loss against the raw labels and calls Train(tokenEmbeddings,
labels), which can mismatch if preprocessing pads/truncates sequences; update
the routine to preprocess/align labels the same way you preprocess inputs (e.g.,
create or call a PreprocessLabels/AlignLabels function or slice labels to match
Forward(...) output length), compute LossFunction.CalculateLoss using the
aligned labels and outputs, use those aligned labels when calling Train, and add
a defensive check that output.Shape equals labels_aligned.Shape and throw a
clear exception if they differ (reference methods: PreprocessTokens, Forward,
LossFunction.CalculateLoss, Train, INERModel<T>.TrainAsync).
In `@src/NER/SpanBased/BiaffineNER.cs`:
- Around line 97-104: CreateNewInstance currently constructs a BiaffineNER
either with a model path or in native-mode without carrying over optimizer
state; make this explicit by adding a short clarifying comment in the
CreateNewInstance method (near UseNativeMode, optionsCopy, and the BiaffineNER
constructors) stating that the optimizer is intentionally not cloned for
native-mode instances (or, if the intended behavior is to preserve optimizer
state, modify the native-mode branch to pass the optimizer/clone it into the new
BiaffineNER). This ensures future maintainers understand why the optimizer isn't
forwarded when creating a native-mode instance.
In `@src/NER/SpanBased/SpanBasedNERBase.cs`:
- Around line 445-454: In ValidateOptions(), you're performing a modulo by
_options.NumAttentionHeads which can be zero and cause a DivideByZeroException;
update ValidateOptions() to explicitly validate that _options.NumAttentionHeads
is > 0 (and throw an ArgumentException with a clear message referencing
NumAttentionHeads) before using it in the HiddenDimension %
_options.NumAttentionHeads check, and keep the existing NumLabels vs LabelNames
length check as-is (or add similar validation for NumLabels if desired).
- Around line 164-195: The TrainAsync implementation in SpanBasedNERBase.cs
calls Forward(PreprocessTokens(tokenEmbeddings)) but passes raw labels into
LossFunction.CalculateLoss and Train, which can lead to mismatched output/target
lengths after truncation/padding; update the training path to preprocess/align
labels the same way as inputs (e.g., add or call a PreprocessLabels/AlignLabels
step on labels or slice/pad labels to match the processed tokenEmbeddings), then
compute loss and call Train using the aligned label tensor, and add a fail-fast
guard before loss calculation (using Forward, PreprocessTokens,
LossFunction.CalculateLoss, Train, and INERModel<T>.TrainAsync as anchors) that
throws a clear exception if output and label vector lengths differ; apply the
same fix to the other training block referenced (lines 277-289).
In `@src/NER/TransformerBased/TransformerNERBase.cs`:
- Around line 213-217: ValidateInputShape currently permits rank-3 (batched)
tensors via INERModel<T>.ValidateInputShape but the subsequent preprocessing
assumes rank-2; update the preprocessing logic in TransformerNERBase to be
rank-aware: if input.Rank == 3 treat the first dimension as batch (keep batch,
seqLen, hiddenDim), otherwise handle rank-2 as [seqLen, hiddenDim]; when rank==3
either iterate per-batch or reshape/flatten only the batch and seq dimensions
consistently and ensure downstream outputs are un-flattened back to [batch,
seqLen, ...]. Adjust any code paths in the preprocessing block (the code
handling seqLen/hiddenDim around the current rank-2-only section) to preserve
batch size and return tensors with the original batch-aware shape.
- Around line 465-474: In ValidateOptions(), guard against zero or negative
attention heads before performing the divisibility check: first validate that
_options.NumAttentionHeads > 0 and throw an ArgumentException with a clear
message referencing NumAttentionHeads when it's invalid, then perform the
existing HiddenDimension % _options.NumAttentionHeads divisibility check; update
messages to reference the relevant symbols (_options.NumAttentionHeads,
_options.HiddenDimension) so the error is deterministic and safe from
divide-by-zero.
- Around line 170-200: The training loop in INERModel<T>.TrainAsync computes
network output from PreprocessTokens(tokenEmbeddings) but computes loss against
raw labels, which can mismatch if PreprocessTokens truncates/pads; modify the
method to validate shapes immediately after
Forward(PreprocessTokens(tokenEmbeddings)) by comparing output shape/length to
the labels vector (or to a labels-preprocessed counterpart), and if they differ
throw a clear exception (or pre-process labels the same way) before calling
LossFunction.CalculateLoss or Train; ensure Train(tokenEmbeddings, labels) is
called with matching-aligned inputs (either pass preprocessed embeddings and
aligned labels or adjust Train to accept the raw vs preprocessed pair) and
include the shape check near Forward/CalculateLoss to fail fast.
---
Duplicate comments:
In `@src/Helpers/LayerHelper.cs`:
- Around line 25642-25646: The code updates currentInputSize =
embeddingDimension + charHiddenDimension as if word+char fusion occurred, but
never emits the fusion layer; fix by inserting a fusion operation (concatenate
char features and word embeddings, then an optional projection to the expected
size) into the sequence produced by LayerHelper (e.g., before the token-level
LSTM that consumes currentInputSize), emit a concrete fusion layer node (concat
+ Linear/Projection) and update currentInputSize to the projection output size
(or keep embeddingDimension if projecting back), and apply the same fix for the
duplicate site where currentInputSize is set at the other occurrence (around the
second reported location) so dimension chaining remains correct.
- Around line 25587-25596: The CreateDefault*Layers methods
(CreateDefaultBiLSTMCRFLayers, CreateDefaultCNNBiLSTMCRFLayers,
CreateDefaultLSTMCRFLayers, CreateDefaultTransformerNERLayers,
CreateDefaultSpanBasedNERLayers) violate the required signature contract by
omitting the first parameter; change their signatures to begin with
NeuralNetworkArchitecture<T> architecture as the first parameter (e.g., public
static IEnumerable<ILayer<T>>
CreateDefaultBiLSTMCRFLayers(NeuralNetworkArchitecture<T> architecture, int
embeddingDimension = 100, ...)), and update any internal references or overloads
to accept and forward this architecture parameter accordingly so the methods
match the shared LayerHelper signature pattern.
- Around line 25598-25610: Add missing public-parameter validation to all
CreateDefault*Layers helper methods (e.g., the BiLSTM-CRF variant and the other
methods referenced) by checking that dropoutRate is within [0,1], and that
maxSequenceLength, charEmbeddingDimension, charHiddenDimension, charCNNFilters,
charCNNKernelSize, intermediateDimension, and spanEmbeddingDimension are
positive (and kernel sizes/filters are integers > 0). Throw
ArgumentOutOfRangeException with nameof(...) and a clear message when a check
fails; mirror the existing pattern used for embeddingDimension, hiddenDimension,
numLabels, and numLSTMLayers in the methods CreateDefault...Layers so each
method validates its own method-specific parameters.
- Around line 25530-25541: The XML docs for several public factory methods are
incomplete; update each CreateDefault*Layers method (including
CreateDefaultBiLSTMCRFLayers, CreateDefaultSpanBasedNERLayers and the
CNN/LSTM/Transformer/Span variants referenced) to include full XML
documentation: add missing <param> entries for useCharEmbeddings,
charEmbeddingDimension, charHiddenDimension (for CreateDefaultBiLSTMCRFLayers),
add the missing <returns> element for CreateDefaultSpanBasedNERLayers, and
ensure every CreateDefault*Layers method has a <remarks> section containing a
<para><b>For Beginners:</b> ...</para> block plus any other required <param>
tags noted in the review; keep descriptions concise and match the existing param
phrasing style used elsewhere in LayerHelper to remain consistent.
- Around line 25549-25559: The documentation and sizing disagree about BiLSTM
output shape: docs claim concatenation (2 * hiddenDimension) but the
implementation uses an additive/merged output (hiddenDimension); inspect
BidirectionalLayer<T> (especially the mergeMode behavior) to determine whether
it concatenates or sums, then make code/comments consistent: if mergeMode
implements concatenation, update CreateDefaultBiLSTMCRFLayers and any
currentInputSize/projection Dense input sizing to use 2 * hiddenDimension and
adjust downstream layer inputs; if mergeMode implements additive merge, update
the XML comments and any references to 2 * hiddenDimension (including the
projection layer description and docs in LayerHelper.cs) to hiddenDimension so
layer dimensions chain correctly between BidirectionalLayer<T>,
CreateDefaultBiLSTMCRFLayers, currentInputSize, and the projection Dense.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (35)
src/Helpers/LayerHelper.cssrc/NER/Options/CNNBiLSTMCRFOptions.cssrc/NER/Options/LSTMCRFOptions.cssrc/NER/Options/SpanBasedNEROptions.cssrc/NER/Options/TransformerNEROptions.cssrc/NER/SequenceLabeling/CNNBiLSTMCRF.cssrc/NER/SequenceLabeling/LSTMCRF.cssrc/NER/SpanBased/BiaffineNER.cssrc/NER/SpanBased/PURENER.cssrc/NER/SpanBased/PyramidNER.cssrc/NER/SpanBased/SpERTNER.cssrc/NER/SpanBased/SpanBasedNERBase.cssrc/NER/SpanBased/TriaffineNER.cssrc/NER/SpanBased/W2NER.cssrc/NER/TransformerBased/BERTNER.cssrc/NER/TransformerBased/BLINKNER.cssrc/NER/TransformerBased/BioBERTNER.cssrc/NER/TransformerBased/ClinicalBERTNER.cssrc/NER/TransformerBased/DeBERTaNER.cssrc/NER/TransformerBased/DistilBERTNER.cssrc/NER/TransformerBased/ELECTRANER.cssrc/NER/TransformerBased/FinBERTNER.cssrc/NER/TransformerBased/InstructionNER.cssrc/NER/TransformerBased/LegalBERTNER.cssrc/NER/TransformerBased/ONNXNER.cssrc/NER/TransformerBased/PromptNER.cssrc/NER/TransformerBased/RELNER.cssrc/NER/TransformerBased/RoBERTaNER.cssrc/NER/TransformerBased/SECBertNER.cssrc/NER/TransformerBased/SciBERTNER.cssrc/NER/TransformerBased/SpanBERTNER.cssrc/NER/TransformerBased/TemplateNER.cssrc/NER/TransformerBased/TinyBERTNER.cssrc/NER/TransformerBased/TransformerNERBase.cssrc/NER/TransformerBased/XLMRoBERTaNER.cs
PubMedBERT is pre-trained from scratch on PubMed abstracts with a domain-specific vocabulary, outperforming BioBERT on all biomedical NER benchmarks. Completes issue #898 item 21 which lists both BioBERT-NER and PubMedBERT-NER. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/NER/TransformerBased/TransformerNERBase.cs:1
- This preprocessing logic is not correct for rank-3 inputs ([batch, seqLen, hiddenDim]) even though
ValidateInputShapeaccepts rank 3. It readsseqLenfromShape[0](batch size) and pads into a 2D tensor, effectively dropping the batch dimension and producing incorrect results. UpdatePreprocessTokensto handle both rank-2 and rank-3 tensors (pad/truncate per batch element and preserve the original rank).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix PreprocessTokens to handle rank-3 [batch, seqLen, embDim] inputs - Add PreprocessLabels to align labels with padded/truncated inputs - Fix TrainAsync: single forward pass (no double computation), add ONNX guard - Fix Train: preprocess labels to match preprocessed input length - Add useCRF parameter to CreateDefaultBiLSTMCRFLayers (UseCRF=false now works) - Fix BiLSTM docs: mergeMode=true does element-wise add, not concatenation - Fix char embedding branch: add fusion projection layer - Change UseCharEmbeddings default to false (model input is token embeddings) - Fix LabelNames setter: defensive clone to prevent shared mutable state - Add OnnxOptions null guard in setter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- CNNBiLSTMCRF: add ONNX guard and single-pass TrainAsync, rank-3 PreprocessTokens, PreprocessLabels helper, ONNX ComputeEmissionScores - TransformerNERBase: ONNX guard, single-pass TrainAsync, rank-3 PreprocessTokens, PreprocessLabels, NumAttentionHeads zero guard - SpanBasedNERBase: ONNX guard, single-pass TrainAsync, rank-3 PreprocessTokens, PreprocessLabels, NumAttentionHeads zero guard - LSTMCRF: ONNX guard, single-pass TrainAsync, rank-3 PreprocessTokens, PreprocessLabels, ONNX ComputeEmissionScores - SpanBasedNEROptions: fix DropoutRate validation to [0,1], add OnnxOptions null guard with backing field, deep copy in constructor - SequenceLabelingNERBase: remove misleading nested entities claim Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This branch was successfully deployed
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
NERNeuralNetworkBase<T>->SequenceLabelingNERBase<T>->BiLSTMCRF<T>INERModel<T>interface,NERModelVariantenum,BiLSTMCRFOptions, andNamedEntityRecognitionModelType entryNew Files (7)
src/Enums/NERModelVariant.cssrc/NER/Interfaces/INERModel.cssrc/NER/NERNeuralNetworkBase.cssrc/NER/SequenceLabeling/SequenceLabelingNERBase.cssrc/NER/Options/BiLSTMCRFOptions.cssrc/NER/SequenceLabeling/BiLSTMCRF.cssrc/Enums/ModelType.csArchitecture
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
API
Documentation