|
| 1 | +# Bug Report: `estimate_complexity()` and `reduceMax` — confirmed issues |
| 2 | + |
| 3 | +> Prepared as a constructive code review. All findings are reproducible with minimal |
| 4 | +> snippets included. No changes to logic are proposed — only the bugs are documented. |
| 5 | +
|
| 6 | +--- |
| 7 | + |
| 8 | +## 1. `ML/src/python/neuralforge/nas/search_space.py` — `estimate_complexity` |
| 9 | + |
| 10 | +### 1a. `UnboundLocalError` when genome starts with an `identity` gene |
| 11 | + |
| 12 | +**Location:** `estimate_complexity()`, the `for gene in architecture.genome` loop |
| 13 | +(lines ~159-179 in the current `master`). |
| 14 | + |
| 15 | +The variables `params` and `flops` are only assigned inside the `if/elif` branches for |
| 16 | +`conv3x3/conv5x3/conv7x7/depthwise/bottleneck`. If the **first** gene in the genome is |
| 17 | +`identity` (or `pooling`), none of those branches execute, so the subsequent line: |
| 18 | + |
| 19 | +```python |
| 20 | +total_params += params # ← NameError / UnboundLocalError |
| 21 | +``` |
| 22 | + |
| 23 | +references a name that has never been bound. |
| 24 | + |
| 25 | +**Reproduction (confirmed by running `search_space.py` directly with a stubbed `torch`):** |
| 26 | + |
| 27 | +```python |
| 28 | +ss = SearchSpace({}) |
| 29 | +genome = [{"type": "identity", "channels": 64, |
| 30 | + "activation": "relu", "use_bn": False, "dropout": 0.0}] |
| 31 | +ss.estimate_complexity(Architecture(genome)) |
| 32 | +# UnboundLocalError: cannot access local variable 'params' |
| 33 | +# where it is not associated with a value |
| 34 | +``` |
| 35 | + |
| 36 | +**Suggested fix:** initialize `params = 0` and `flops = 0` at the top of the loop body |
| 37 | +(before the `if` chain), so that every code path has a defined value. |
| 38 | + |
| 39 | +--- |
| 40 | + |
| 41 | +### 1b. Stale `params`/`flops` carried across genes → double-count |
| 42 | + |
| 43 | +Even when the first gene is a conv (so no `UnboundLocalError`), the variables are never |
| 44 | +reset between loop iterations. An `identity` gene after a `conv3x3` therefore re-adds the |
| 45 | +**previous conv's** `params` and `flops`. |
| 46 | + |
| 47 | +**Reproduction (confirmed by running):** |
| 48 | + |
| 49 | +```python |
| 50 | +genome = [ |
| 51 | + {"type": "conv3x3", "channels": 64, "activation": "relu", |
| 52 | + "use_bn": False, "dropout": 0.0}, |
| 53 | + {"type": "identity", "channels": 64, "activation": "relu", |
| 54 | + "use_bn": False, "dropout": 0.0}, |
| 55 | +] |
| 56 | +r = ss.estimate_complexity(Architecture(genome)) |
| 57 | +# r == {'params': 3456, 'flops': 173408256} |
| 58 | +# correct params for a single 3x64x3x3 conv = 3*64*3*3 = 1728 |
| 59 | +# → over-counted by exactly 2× |
| 60 | +``` |
| 61 | + |
| 62 | +**Suggested fix:** same as 1a — set `params = 0; flops = 0` at the top of each loop |
| 63 | +iteration. |
| 64 | + |
| 65 | +--- |
| 66 | + |
| 67 | +### 1c. `identity` with a channel change silently drops the 1×1 conv parameters |
| 68 | + |
| 69 | +In `build_model()`, an `identity` gene whose input and output channel counts differ is |
| 70 | +implemented as a `nn.Conv2d(current, out, 1)` (a 1×1 conv with `in*out` parameters). |
| 71 | +`estimate_complexity()` has **no branch** for this case and reports the parameters as |
| 72 | +whatever stale value was left over (see 1b) or 0 (see 1a). |
| 73 | + |
| 74 | +**Reproduction (confirmed by running):** |
| 75 | + |
| 76 | +```python |
| 77 | +genome = [ |
| 78 | + {"type": "conv3x3", "channels": 64, "activation": "relu", |
| 79 | + "use_bn": False, "dropout": 0.0}, |
| 80 | + {"type": "identity", "channels": 128, "activation": "relu", |
| 81 | + "use_bn": False, "dropout": 0.0}, |
| 82 | +] |
| 83 | +r = ss.estimate_complexity(Architecture(genome)) |
| 84 | +# The identity step inserts a Conv2d(64, 128, 1) → 64*128 = 8192 params |
| 85 | +# correct total = 1728 + 8192 = 9920 |
| 86 | +# reported = 3456 (stale conv value re-added, 1×1 conv params missing) |
| 87 | +``` |
| 88 | + |
| 89 | +**Suggested fix:** add an `elif gene['type'] == 'identity':` branch that computes |
| 90 | +`params = current_channels * out_channels` when the two differ, else 0. |
| 91 | + |
| 92 | +--- |
| 93 | + |
| 94 | +## 2. `ML/src/cuda/kernels.cu` — `reduceMax` |
| 95 | + |
| 96 | +### 2a. `atomicMax` on `__float_as_int` is only correct for non-negative floats |
| 97 | + |
| 98 | +**Location:** `reduceMax`, line ~170. |
| 99 | + |
| 100 | +```cuda |
| 101 | +if (tid == 0) { |
| 102 | + atomicMax((int*)output, __float_as_int(sdata[0])); |
| 103 | +} |
| 104 | +``` |
| 105 | + |
| 106 | +`__float_as_int` reinterprets the bit pattern of a `float` as an `int`. For **non-negative** |
| 107 | +floats the integer ordering matches the float ordering (both are monotonic from 0 upward), |
| 108 | +so `atomicMax` on the int representation works. |
| 109 | + |
| 110 | +For **negative** floats the sign bit is set, so the integer value is negative and the |
| 111 | +ordering is **reversed** relative to float order. Example: |
| 112 | + |
| 113 | +| float | `__float_as_int` (hex) | as signed int | |
| 114 | +|--------|------------------------|---------------| |
| 115 | +| −1.0 | `0xBFC00000` | −1088259840 | |
| 116 | +| −0.5 | `0xBF000000` | −1072693248 | |
| 117 | + |
| 118 | +`-1.0 < -0.5` in float, but `-1088259840 < -1072693248` is also true here — so for two |
| 119 | +negatives it happens to work. However, mixing a negative and a positive value breaks: |
| 120 | + |
| 121 | +| float | as signed int | |
| 122 | +|--------|---------------| |
| 123 | +| −0.1 | ≈ -1056964608 | |
| 124 | +| +0.1 | ≈ 1027384934 | |
| 125 | + |
| 126 | +`atomicMax` correctly picks `+0.1`. But the initial value of `output` (host-allocated, |
| 127 | +usually 0) is compared against the first block's negative result: `atomicMax(0, -1056964608)` |
| 128 | +returns **0**, silently discarding the true maximum if all values are negative. |
| 129 | + |
| 130 | +**Reproduction:** call `cuda_reduce_max` on an all-negative array; the returned maximum |
| 131 | +will be `0.0f` instead of the (correct) largest negative value, because the host-side |
| 132 | +`output` is initialised to 0 and `atomicMax` never overwrites it with a smaller int. |
| 133 | + |
| 134 | +**Suggested fix:** either (a) initialise `output` to `__int_as_float(0xFF800000)` (−∞) |
| 135 | +on the host before the kernel, or (b) run a second reduction kernel over the per-block |
| 136 | +results using `fmaxf` instead of `atomicMax`. |
| 137 | + |
| 138 | +--- |
| 139 | + |
| 140 | +## Summary |
| 141 | + |
| 142 | +| # | File | Function | Severity | Symptom | |
| 143 | +|---|------|----------|----------|---------| |
| 144 | +| 1a | `search_space.py` | `estimate_complexity` | High | `UnboundLocalError` crash on `identity`-first genome | |
| 145 | +| 1b | `search_space.py` | `estimate_complexity` | Medium | Params over-counted ×2 for `conv → identity` pairs | |
| 146 | +| 1c | `search_space.py` | `estimate_complexity` | Medium | 1×1 conv params in `identity` silently omitted | |
| 147 | +| 2a | `kernels.cu` | `reduceMax` | Medium | Returns 0 instead of true max when all inputs are negative | |
0 commit comments