Warrior_EA/REFACTOR_NOTES.md

Refactor + audit — branch refactor/solid-dry

Baseline before any change: 0 errors, 0 warnings (commit 4f28165). Every commit on this branch compiles 0/0 independently and was verified before being committed.

Compiling from C:\Users\admin\Documents\Workspaces\Warrior_EA still fails with error 313: invalid resource path 'Network.cl'; the build is done by copying the tree into MQL5\Experts\ first, per the documented workaround.

---

A. Real bugs found and fixed

A1. Non-atomic sidecar writes — .stats, .arrows, .cfg

CNet::Save staged the .nnw through a temp file + rename because FileOpen(FILE_WRITE) truncates on open. The three sidecars written beside it did not get that treatment.

1. An interrupted write published a truncated sidecar. Worst case is .cfg: LoadAndCompareTopologyConfiguration() reads a short file as a mismatch, which discards the trained model and restarts from era 0.
2. The reader-side share flags were inert. Windows sharing is a mutual contract — a writer opened with no FILE_SHARE_* blocks every concurrent open regardless of the reader's flags. All three read paths carry FILE_SHARE_READ|FILE_SHARE_WRITE precisely so a tester agent can read them while a live chart runs; an exclusive writer on the same path defeated that.

Fixed by extracting CNet::Save's pattern into System\AtomicFile.mqh (AtomicWriteBegin / AtomicWriteEnd) and routing all four writers through it. That also encodes the FileMove trap once: the destination location comes from FILE_COMMON inside the 4th argument and is not inherited from the source — getting it wrong silently moves the file to the wrong sandbox.

A2. Handle leak on every .cfg write-error path

SaveTopologyConfiguration had 13 copy-pasted 6-line error blocks that each return false without FileClose(handle). Collapsed into one ok-chain that closes exactly once. The on-disk field order and types are unchanged — asserted mechanically during the rewrite — so existing .cfg files still load.

A3. SaveChartSignals documented a guarantee it did not provide

The comment said pruning runs only after a successful write ("a failed write above leaves both the file AND the chart untouched"), but no write result was ever checked, so a partial write still deleted the chart objects and lost those arrows in both places. Results are checked now.

A4. Topology drift between LSTM and HYBRID

CSignalHYBRID guarded the LSTM step with MathMax(1, historyBars/2); CSignalLSTM divided unguarded. A historyBars of 1 gave two different steps for what the comments describe as the same layer. Unified on the guarded form.

A5. Descriptor leak in topology construction

On a failed topology.Add() the CLayerDescription was neither owned by the array nor deleted. Fixed in the extracted stages.

---

B. Dead code removed

• CNet::SaveCheckpoint / CNet::LoadCheckpoint (123 lines). Superseded by the in-memory CaptureWeights/RestoreWeights pair; Network.mqh:1312 already said so. Zero call sites — every remaining mention was a comment. The five comments that referenced them were reworded rather than left dangling.
• CExpertSignalCustom::CheckForDuplicateTrade / FindLastTradeIndex / UpdateTradeStatusAndExit — declared, never defined anywhere, never called. They only made it look as though duplicate-trade detection existed.

Not removed, deliberately

• TCIndexOk and friends in System\TradeChecks.mqh are an intentional guard library, documented against MQL5 runtime error codes. Unused entries are API.
• getPrevOutIndex is one member of a symmetric get*Index() accessor family.
• Stale worktree .claude/worktrees/agent-abfbb5952bdcac100 + branch worktree-agent-abfbb5952bdcac100: fully merged into main, zero unique commits. Removal was blocked by the permission classifier (forced delete), so it is left for you:

  git worktree remove .claude/worktrees/agent-abfbb5952bdcac100 --force
  git branch -D worktree-agent-abfbb5952bdcac100
  

Checked and clean

A scan for new-into-local with early returns flagged 17 candidates; all were inspected and all are correct — every failure path in the neuron-construction switch deletes both the neuron and the partially built layer. Allocation discipline in this codebase is good. Likewise, every FileOpen(...FILE_READ...) already carried the required share flags: zero violations of that project rule.

---

C. Structure — CExpertSignalAIBase split by responsibility

The class was 8216 lines: declaration plus 87 method bodies covering training, labelling, features, persistence, chart drawing, online learning, the GA tuner and inference, all in one file. Train() alone is 1492 lines.

Bodies moved to Expert\AIBase\, included at the bottom of the original after the class declaration:

file	lines	holds
Training.mqh	1607	era loop, plateau ladder, checkpoint select, deploy
Features.mqh	1093	indicator creation + per-bar input feature vector
ChartUI.mqh	634	arrows, arrow persistence, status panel, cleanup
Persistence.mqh	492	.stats/.cfg sidecars, CPU-inference validation, copy
OnlineLearning.mqh	461	live continual learning, EMA shadow, OOS simulator
Labels.mqh	309	ZigZag pivot labels, async label-cache prebuild
AutoTune.mqh	275	genetic tuner (population, crossover, halving)
Inference.mqh	235	softmax, prior calibration, class priors
ExpertSignalAIBase.mqh	8216 → 3131	declaration + topology build

This was verified to be a pure relocation, mechanically rather than by eye: HEAD's file reconstructed from the eight partials plus the surviving remainder is byte-identical to HEAD, span for span. No declaration moved, no signature changed, no code rewritten — behaviour is unchanged by construction.

Topologies are now compositions

AddConvPoolStage() / AddLstmStage() on the base class; the three overrides became:

CONV   = AddConvPoolStage
LSTM   = AddLstmStage
HYBRID = AddConvPoolStage && AddLstmStage

HYBRID's "matches the standalone CONV front-end exactly, then adds LSTM" is now enforced by construction instead of by comment — which is what let A4 drift in the first place.

---

D. The MLP-slowness investigation — correcting what I told you

I previously said MLP is slowest because its first dense layer is the largest weight matrix. The arithmetic does not support that as an explanation for a 10× gap. Parameter counts at the shipping defaults (ind_Periods=20, 21 features → input 420, InitialNeurons=500, RF_70, MinNeurons=20, ConvFilters=16, LstmHidden=32, pool 3/2):

topology	first dense	total weights
MLP 3 dense	420→500 = 210,500	~292,600
MLP 4 dense	420→500 = 210,500	~293,400
CONV 2 dense	160→500 = 80,000	~156,000
LSTM 2 dense	33→500 = 16,500	~150,100

1. MLP_3L and MLP_4L are within 0.3% of each other. The 4th dense layer is 45→20 (900 weights) and it shrinks the output layer's fan-in. Nothing about topology can make 3L 10× slower than 4L.
2. MLP vs CONV/LSTM is ~2×, not 10×.

Your two observations together (OpenCL: all similar; CPU DLL: MLP 10× slower) point at CPU DLL dispatch, not network shape. On OpenCL these matrices are far too small to saturate the device, so every topology is dispatch-latency-bound and should look identical — which is exactly what you see. The CPU tier is the only one where real per-weight throughput shows, and there the gap is ~5× larger than the weights justify.

Not diagnosed. This needs profiling, not more static reading. Concrete next step: log wall-clock per feedForward/backProp call per layer index on the CPU tier, MLP_3L vs MLP_4L. If one layer transition dominates, it is a dispatch/threading bug in WarriorCPU.cpp. If cost is spread evenly, suspect thread-pool oversubscription — SetCpuLoadPercent is applied per network, and a run builds several (main + EMA shadow + sim/self-check clones).

One relevant note found while checking: AIType selects exactly one architecture (EnablePAI/CONV/LSTM/HYBRID are mutually exclusive, derived from it at Warrior_EA.mq5:805-808), so a four-topology comparison is four EA instances, each with its own CPU-DLL thread pool on the same box.

---

E. Still to do (not done here)

• Rebuild both DLLs (build.bat / build_cpu.bat). The .cpp sign-flip removal from the previous session requires it, and that rebuild also picks up the uncommitted D3D12 barrier fix that clears the DirectML LSTM feedForward errors.
• Redeploy the .ex5 and retrain.
• The MLP CPU-DLL profiling in §D.

---

2026-07-29 (later): root cause of the collapse, and the architectural gap

F. A .nnw pins the ARCHITECTURE, not just the weights — fixed in 1aa7df9

CNeuronBaseOCL::Save writes (int)activation per neuron; Load reads it straight back. So the activation chosen in BuildFreshTopology() only ever reached a brand-new topology — every reload restored the file's value and the next save wrote it back out. A wrong value could never heal on its own, while the source read as though it were already fixed.

That is how five models kept training with an unbounded NONE classification head for a full day after the 07-28 revert to SIGMOID. Confirmed by parsing the binaries directly (parser recipe is in the memory note):

848cb42c.nnw  era=5  layer 4: BaseOCL act=NONE     out=3   <- was training
2e754b43.nnw  era=5  layer 5: BaseOCL act=NONE     out=3   <- was training
9d16f2c6.nnw  era=0  layer 4: BaseOCL act=SIGMOID  out=3   <- genuinely reset

In the log it showed as negative OOS raw out values — impossible under a sigmoid — escalating to a 4.14e13 logit spread with all three classes numerically identical (input-independent output) and balanced accuracy pinned on the 33.3% one-class floor.

Second half of the trap: reset-weights only cleans the current fingerprint. Changing any fingerprinted input moves the filename, and the EA then adopts whatever stale .nnw already sits at the new name — resurrecting an obsolete architecture the reset had just eliminated. Reset after an input change, never before.

Fix: OutputLayerActivation() is now the single source of truth, called by both BuildFreshTopology() and a new load-time repair that re-asserts it and logs loudly when the file disagreed.

G. Batch normalization — 30206cb, 65dc1bd

Audit against references/MQL5/Experts/NeuroNet_DNG/NeuroNet.mqh and references/neuronetworksbook.pdf (extract with pdftotext -layout) found two genuine gaps, and they turn out to be the same gap:

1. No inter-layer normalization (book ch. 6.1, reference class CNeuronBatchNormOCL). Inputs are ATR-normalized thoroughly, but every hidden stage is PRELU — the sigmoid head was the only bounded stage in the forward path. The observed collapse ordered exactly by depth, which is the signature of internal covariate shift.
2. No dedicated SoftMax layer. The reference runs Dense(None) -> CNeuronSoftMaxOCL -> CCE with the full Jacobian; this engine runs Dense(SIGMOID) -> x6 -> softmax with the gradient short-circuited to (target - softmax). Defensible as an anti-saturation heuristic, but it caps expressible logits at [0,6] and forces saturation to reach confidence.

They are linked: the 07-27 attempt to unbound the head failed because nothing upstream constrained scale. Batch norm before the head is the precondition for ever retrying that (and would also need CLASS_LOGIT_SCALE -> 1.0 and BIAS_MAGNITUDE -> ~0.5).

Implemented as AI/NeuronBatchNorm.mqh, host-side rather than a fourth copy of a kernel across Network.cl + WarriorCPU.cpp + WarriorDML.cpp — the math is elementwise O(n), so this way all four tiers behave identically, no DLL rebuild is needed, and the backends cannot drift apart. Statistics are exponential moving (training is pure online SGD; there is no mini-batch). gamma/beta are excluded from weight decay.

Verified without a live run: analytic gradients match finite differences to 1.5e-7 relative over 200 random cases, and a faithful Python port of the whole forward/backward chain collapses to the 33.3% floor by era 4 without the layer and holds 36-43% with it.

Do not port the reference's batch-norm kernels verbatim if that is ever revisited: UpdateBatchOptionsAdam reads the momentum slots (+5..6) where it needs the second-momentum slots (+7..8), and carries the same sign-agreement gate this project already removed as a downward ratchet.