context/CODE_REVIEW.md

# Deep Code Review – Photosynthesis Prediction Model

**Scope:** `config/settings.py`, `src/*.py`  
**Date:** 2025-02

---

## 1. Executive Summary

The OOP architecture matches the plan and is generally sound. **Critical issues** to fix: (1) IMS `resample_to_15min` receives DataFrames whose first channel may have `timestamp_utc` as index, not column; (2) `Preprocessor.merge_ims_with_labels` can produce duplicate timestamp columns and misalign when labels are not index-aligned; (3) `FarquharModel.compute_all` uses row-wise Python loop and can leak NaN. **Medium** issues: missing validation (paths, token, NaN handling), IMS timezone handling, and preprocessor edge cases. **Low**: type hints, docstrings, and minor robustness.

---

## 2. config/settings.py

| Finding | Severity | Notes |
|--------|----------|--------|
| No validation that paths exist | Low | `SENSORS_WIDE_PATH` may not exist; code fails at load time. Acceptable but could add a `validate_paths()` or document. |
| `TRAIN_RATIO` used with `or` in Preprocessor | Low | `train_ratio or settings.TRAIN_RATIO` treats `0.0` as falsy; use `train_ratio if train_ratio is not None else settings.TRAIN_RATIO` if 0 is ever valid. |

**Verdict:** Good; no critical bugs.

---

## 3. src/sensor_data_loader.py

| Finding | Severity | Notes |
|--------|----------|--------|
| `read_csv(..., usecols=lambda c: c in use_cols)` | Medium | If the CSV has no `time` column or a column is missing, `read_csv` may error or drop columns silently. Consider validating columns after load. |
| Missing columns not reported | Low | When requested columns are absent, pandas may omit them; caller gets incomplete DataFrame. |
| `filter_daytime` with NaN in PAR | Low | `df[par_column] > par_threshold` yields False for NaN; those rows are dropped. Document or add `dropna(subset=[par_column])` explicitly. |

**Verdict:** Solid. Add a post-load check that required columns exist and optionally log missing ones.

---

## 4. src/ims_client.py

| Finding | Severity | Notes |
|--------|----------|--------|
| **`resample_to_15min` assumes column** | **Critical** | `fetch_channel` returns a DataFrame with `timestamp_utc` as **index** (line 83). `fetch_all_channels` builds `out` by joining such DataFrames and then `reset_index()`, so the final `out` has `timestamp_utc` as a column. So after `fetch_all_channels` we're OK. But `resample_to_15min` is also called with the result of `fetch_all_channels` which has the column. So actually OK. Double-check: `load_cached` returns df with column; `fetch_and_cache` calls `fetch_all_channels` then `resample_to_15min` – and `fetch_all_channels` does `reset_index()`, so column exists. **Verdict:** OK as written. |
| Empty token | Medium | If `IMS_API_TOKEN` is missing, `fetch_channel` will call API with empty token and get 401. Add an explicit check in `fetch_channel` or `__init__` and raise a clear error. |
| IMS returns Israel time | Medium | Doc says IMS returns Israel time; we pass to `pd.to_datetime(..., utc=True)`. If the string has no timezone, pandas treats it as local (or naive). We should parse as Israel and convert to UTC, or document that we assume UTC. |
| No retries / rate limiting | Low | Single request; 60s timeout. For long ranges the API may throttle; consider retries and chunking (as in plan). |
| `fetch_all_channels` first frame has index, no column | Critical | First iteration: `out = df` where `df` is from `fetch_channel` – so `out` has **index** `timestamp_utc`, no column named `timestamp_utc`. Then `out.join(df, ...)` keeps index. So after the loop, `out.reset_index()` creates column `timestamp_utc`. So we're good. |
| `load_cached` and CSV timestamp | Low | After `read_csv`, column is object; we convert with `pd.to_datetime(..., utc=True)`. Good. |

**Verdict:** Add token validation; clarify or fix timezone handling for IMS datetime.

---

## 5. src/farquhar_model.py

| Finding | Severity | Notes |
|--------|----------|--------|
| **`compute_all` row-wise loop** | **Medium** | `df.iterrows()` is slow and not vectorized; any NaN in a row propagates (e.g. `row[par_col]` can be NaN). We should vectorize or use `apply` and handle NaN explicitly (e.g. skip or fill). |
| **NaN propagation** | **Medium** | `calc_photosynthesis` and helpers don't check for NaN; numpy will propagate them. So `compute_all` can return a Series full of NaN if one input column has NaNs. Add NaN handling (e.g. return NaN for that row and document). |
| Ko units and OI | Low | Plan: `Ko = exp(20.30 - 36380/(R*Tk))`; code multiplies by 1000 "to match OI". OI is 210 mmol/mol. Formula units vary by source; document or cite so Ko/OI consistency is clear. |
| Vcmax/Jmax above Topt | Low | Simplified Gaussian decline above Topt; not the full Greer & Weedon equation. Document as simplified. |
| `_ci_from_ca` and gs_factor | Low | Stomatal model is heuristic (VPD/CWSI scaling). Document as simplified. |
| Division by zero | Low | In `calc_photosynthesis`, `ci + Kc*(1+OI/Ko)` could in theory be 0 for extreme T; clip or guard if needed. |

**Verdict:** Fix NaN handling and consider vectorization or explicit NaN-in → NaN-out in `compute_all`.

---

## 6. src/preprocessor.py

| Finding | Severity | Notes |
|--------|----------|--------|
| **merge when labels not index-aligned** | **Critical** | When `timestamp_index_labels=False`, we do `merged["A"] = labels.values[: len(merged)]`. This assumes `labels` and `ims_df` are **ordered identically** by time and that `len(labels) >= len(merged)`. If IMS and labels have different timestamps (inner join not used), alignment is wrong. When `timestamp_index_labels=True`, inner merge is correct. Recommendation: deprecate or remove the `timestamp_index_labels=False` path, or require an explicit timestamp column in labels and merge on it. |
| Duplicate timestamp columns after merge | Low | After inner merge we may have both `timestamp_utc` (from IMS) and the label index column (e.g. `time`). Consider dropping the duplicate: `merged.drop(columns=[ts_lab], inplace=True)` if `ts_lab != timestamp_col_ims`. |
| `temporal_split` return contract when n>=len(df) | Medium | When `n >= len(df)` we return `X, y, empty, empty` – so "train" is full dataset, test is empty. Caller may not expect that. Document or return a clear sentinel. |
| `int(len(df) * self.train_ratio)` | Low | For very small df, n can be 0; we already return early for `n <= 0`. Good. |
| `fit_transform_train` overwrites scaler | Low | Each call fits a new scaler; no way to "refit" explicitly. Acceptable. |

**Verdict:** Fix merge logic when labels are not index-based; document or adjust temporal_split when test is empty.

---

## 7. src/predictor.py

| Finding | Severity | Notes |
|--------|----------|--------|
| `get_feature_importance` | Low | Uses `feature_names_in_` (sklearn 1.0+). For older sklearn, fallback to `list(range(len(imp)))` is correct. |
| `plot_results` and `self.results` | Low | If `evaluate` was never called, `self.results` is empty and plot is no-op. Document. |
| R2 when variance is zero | Low | If `y_test` is constant, `r2_score` can be negative or undefined; sklearn returns 0 or a value. Acceptable. |
| Unused import `seaborn` | Low | Remove if not used. |

**Verdict:** Minor cleanups only.

---

## 8. src/__init__.py

| Finding | Severity | Notes |
|--------|----------|--------|
| Lazy `__getattr__` | Low | Works; avoids pulling config/requests on import. |
| No `__dir__` | Low | `dir(src)` won't list the lazy attributes; could add `__dir__` returning `__all__` for better discoverability. |

**Verdict:** Good.

---

## 9. Cross-Cutting and Data Flow

| Finding | Severity | Notes |
|--------|----------|--------|
| Sensor timestamp vs IMS timestamp | Medium | SensorDataLoader uses column `time` (from sensors_wide); IMS uses `timestamp_utc`. For merge, Preprocessor expects labels with datetime index or a column. Ensure sensor data index (or column) is set to the same timezone (UTC) and format when building labels for merge. |
| No end-to-end test | Medium | A minimal script or test that runs load → Farquhar → merge → split → train → evaluate would catch integration bugs. |

---

## 10. Recommended Fixes (Priority Order)

1. **Preprocessor.merge_ims_with_labels:** When `timestamp_index_labels=False`, do not align by position; require a timestamp column in labels and merge on it, or drop this branch and require index-based labels.
2. **IMSClient:** Validate `self.token` in `__init__` or at first fetch and raise a clear error if missing.
3. **FarquharModel.compute_all:** Handle NaN (e.g. skip row or set A=NaN); consider vectorizing or using `apply` with a wrapper that catches NaN.
4. **Preprocessor:** After merge, drop the redundant timestamp column if `ts_lab != timestamp_col_ims`.
5. **SensorDataLoader.load:** After `read_csv`, check that required columns (at least timestamp and Stage 1) are present and log or raise if not.
6. **predictor:** Remove unused `seaborn` import; add `__dir__` to `src/__init__.py` for discoverability.

---

## 11. Summary Table

| Module | Critical | Medium | Low |
|--------|----------|--------|-----|
| config/settings.py | 0 | 0 | 2 |
| sensor_data_loader.py | 0 | 1 | 2 |
| ims_client.py | 0 | 2 | 2 |
| farquhar_model.py | 0 | 2 | 4 |
| preprocessor.py | 1 | 1 | 3 |
| predictor.py | 0 | 0 | 3 |
| __init__.py | 0 | 0 | 1 |

**Total: 1 critical, 6 medium, 17 low.**

---

## 12. scripts/run_pipeline.py and app.py (post-todo completion)

| Finding | Severity | Notes |
|--------|----------|--------|
| run_pipeline: Stage 1 labels index | Low | Labels index set to floor 15min and aggregated; aligns with IMS for merge. Good. |
| run_pipeline: Stage 2 requires overlap | Low | If IMS cache and sensor date ranges don't overlap, merge is empty; message added. |
| run_pipeline: global `pd` | Low | `import pandas as pd` at module level; used in run_stage1 and validate_stage1. |
| app.py: run_stage1/run_stage2 buttons | Low | Buttons trigger pipeline; state not persisted across reruns. Acceptable. |
| app.py: paths in sidebar | Low | Shows config paths; no secrets. Good. |

**Verdict:** Pipeline and app are consistent with the plan; no critical issues.

---

## 13. Applied Fixes (post-review)

- **Preprocessor:** Drop duplicate timestamp column after merge when `ts_lab != timestamp_col_ims`; when `timestamp_index_labels=False`, use labels.index for merge if present to avoid position-based alignment. `train_ratio`: use `None` default and `if train_ratio is None else train_ratio` so `0.0` is valid.
- **IMSClient:** Validate token in `__init__` and raise `ValueError` if missing.
- **FarquharModel.compute_all:** Check for NaN in required inputs per row and append NaN to output; catch TypeError/ZeroDivisionError/ValueError and append NaN.
- **SensorDataLoader.load:** After `read_csv`, raise `ValueError` with missing column names if any requested column is absent.
- **predictor:** Removed unused `seaborn` import.
- **src/__init__.py:** Added `__dir__` returning `__all__` for better discoverability.

---

## 14. Growing-season filter (Oct–April excluded)

| Finding | Severity | Notes |
|--------|----------|--------|
| config.GROWING_SEASON_MONTHS | Low | (5,6,7,8,9) = May–September; single source of truth. |
| run_stage1: filter by month after daytime | Low | Only rows with month in GROWING_SEASON_MONTHS are used for A; labels and Stage 2 are thus growing-season only. No change needed in Stage 2 merge. |
| Hemisphere | Low | Assumes Northern Hemisphere (Israel). Document if reused elsewhere. |

**Verdict:** Correct; reduces label count to active season only and improves Stage 2 relevance.

---

## 15. Applied fixes (verified)

Verified in the codebase as of 2026-02. The following fixes from section 13 are present:

| Fix | Status | Location |
|-----|--------|----------|
| Preprocessor: drop duplicate timestamp after merge | Done | `preprocessor.py` 48–49, 59–61 |
| Preprocessor: merge on timestamp when `timestamp_index_labels=False` and labels have index | Done (partial) | When labels have a named index, merge is on timestamp; else position-based alignment still used (lines 62–64) |
| Preprocessor: `train_ratio` default `None`, use `if train_ratio is None else train_ratio` | Done | `preprocessor.py` 19, 21 |
| IMSClient: validate token in `__init__`, raise `ValueError` if missing | Done | `ims_client.py` 38–41 |
| FarquharModel.compute_all: NaN handling and exception → NaN | Done | `farquhar_model.py` 184–185, 193 |
| SensorDataLoader.load: raise `ValueError` with missing column names | Done | `sensor_data_loader.py` 61–64 |
| predictor: remove unused `seaborn` | Done | Not present in `predictor.py` |
| `src/__init__.py`: add `__dir__` returning `__all__` | Done | `__init__.py` line 14 |

---

## 16. Fixes applied 2026-02-17

The following issues identified in the full code + design review (2026-02-17) have been resolved:

| Fix | Severity | Location | Change |
|-----|----------|----------|--------|
| **Preprocessor position-based merge fallback removed** | Critical | `preprocessor.py:52–64` | The `else` branch that silently aligned labels to IMS rows by position (`labels.values[:len(merged)]`) — which produces incorrect joins when row counts differ — has been replaced with an explicit `ValueError`. All callers use `timestamp_index_labels=True`; the dead code path is no longer reachable. |
| **`use_container_width=True` deprecated API removed** | Medium | `app.py` (~50 occurrences) | Streamlit 1.54 deprecates `use_container_width=True`. All occurrences replaced: `st.image` calls now use `width='stretch'` (equivalent behaviour, since the old default for images was `width='content'`); `st.plotly_chart` and `st.dataframe` calls have the argument removed (their default is already `width='stretch'`). |

---

## 17. Remaining work

**Medium (not yet applied)**

- **IMS timezone:** Doc states IMS returns Israel time; code uses `pd.to_datetime(..., utc=True)`. Either parse as Israel and convert to UTC, or document that input is assumed UTC. *(Note: `ims_client.py` was updated to correctly localize to `Asia/Jerusalem` then convert to UTC; verify this is actually called on the datetime string.)*
- **FarquharModel.compute_all:** Still uses `df.iterrows()`; vectorization or `apply` with NaN-safe wrapper would improve performance for large datasets (NaN handling is already correct).
- **Preprocessor.temporal_split:** When `n >= len(df)` the function returns full dataset as train and empty test; document this contract or return a clear sentinel.
- **No automated tests:** Add at minimum: `test_farquhar.py` (known inputs, NaN handling, ci floor), `test_preprocessor.py` (merge, split, empty edge cases, ValueError for position-based path), `test_solar_geometry.py` (tracker tilt scalar output, shadow mask shape).
- **app.py monolith:** 2568 lines, all 5 tabs inline. Extract tab content into a `tabs/` package for maintainability.

**Low (optional)**

- `compute_face_shading` unbound locals: initialize `east_sunlit = 0.0` and `west_sunlit = 0.0` before conditional branches (currently safe in practice but fragile).
- No-op rename in `ims_client.py:141`: `df.rename(columns={c: c ...})` is an identity operation; remove it.
- Unused `humidity_col` parameter in `FarquharModel.compute_all`: remove or document.
- Module-level singletons in `tracker_optimizer.py`: `_model` and `_shadow` are created on import; acceptable but note this if config changes at runtime.
- Path validation in `config/settings.py`; `filter_daytime` NaN in PAR; Ko/OI units documentation; division-by-zero guard in Farquhar; etc. See sections 2–9 for the full list.

---

## 18. src/chatbot/guardrails.py (added 2026-03-09)

Anti-hallucination guardrails for the vineyard advisor chatbot.

| Component | Role | Notes |
|-----------|------|-------|
| `QueryClassifier` | Regex-based query routing | Tags queries as data/knowledge/greeting/ambiguous. Data queries force tool calls. Generic "what is" without domain keyword → ambiguous (avoids false positives). |
| `ResponseValidator` | Deterministic post-response rule checks | 5 rules: no_shade_before_10 (block), no_shade_in_may (block), temperature_transition (warn <28°C), no_leaves_no_shade_problem (warn), no_shading_must_explain (warn). Block overrides response text. |
| `estimate_confidence` | Data freshness → confidence level | high (<30min), medium (30-120min), low (>120min or no tool), insufficient_data (tool failed). Computed results (FvCB, sim) → high. |
| `tag_tool_result` | Source metadata injection | Adds `_source` (human-readable), `_data_age_minutes`, `_freshness_warning` (if >60min) to tool result dict before Gemini Pass 2. |

**Verdict:** Solid first layer. Heuristic text matching in `_text_recommends_shading` could be fooled by unusual phrasing; consider LLM-based classification for edge cases in a future iteration. Query classifier has a good balance of specificity (domain keywords) vs generality (generic question words treated as ambiguous).

---

## 19. src/chatbot/vineyard_chatbot.py (updated 2026-03-09)

| Change | Notes |
|--------|-------|
| `ChatResponse` extended | New fields: `confidence`, `sources`, `caveats`, `rule_violations`. Backward-compatible (all have defaults). |
| `chat()` flow upgraded | 7-step pipeline: classify → Gemini Pass 1 → force tool if data query → dispatch + tag → Gemini Pass 2 → validate → confidence. |
| System prompt strengthened | Added: "NEVER invent sensor readings", "MUST call a tool for data questions", "cite source and timestamp", "If data unavailable, say so — do NOT estimate". |
| `_get_validation_context()` | Gathers hour, month, stage_id, temp_c for rule validation. Uses `zoneinfo` for Israel time. |

**Verdict:** Well-integrated. The re-prompt mechanism (force tool call) adds one extra Gemini call for data queries where the LLM skips the tool — acceptable latency cost for reliability.

---

## 20. src/energy_budget.py (created 2026-03-10)

| Finding | Severity | Fix |
|---------|----------|-----|
| `compute_weekly_plan` month-end calc overflowed for month=12 (→ month 13) | Bug | Added December guard before the `month+1` path |
| `compute_daily_plan` block weights assumed `NO_SHADE_BEFORE_HOUR=10`; transition block `(10,11)` hardcoded | Medium | Made `transition_end = max(NO_SHADE_BEFORE_HOUR+1, 11)` dynamic |
| `_energy_analytical` loops 15k timestamps in Python | Low | Acceptable for one-shot season planning |
| `spend_slot` doesn't validate negative amounts | Low | Internal API; callers are trusted |

**Verdict:** Clean hierarchical design. Budget math is correct: 920 kWh potential → 46 kWh (5%) → monthly/weekly/daily/slot. Both bugs fixed.

---

## 21. src/command_arbiter.py (created 2026-03-10)

| Finding | Severity | Fix |
|---------|----------|-----|
| `AstronomicalTracker.get_angle` passed naive datetime to pvlib (warns on tz) | Medium | Added `tz_localize("UTC")` fallback for naive timestamps |
| `should_move` uses `pd.Timedelta` for `datetime` comparison | Low | Works; pd dependency already imported |
| `arbitrate` mutates `decision.source` on dispatch | Low | Intentional; overrides "stable"/"initial" with actual priority source |

**Verdict:** Priority stack is clear and correct. Hysteresis correctly prevents sub-slot jitter while allowing safety-critical overrides (weather/harvest) to bypass the filter. All fallbacks default to θ_astro (zero energy cost).

---

## 22. src/spectral_aggregator.py (created 2026-03-10)

| Finding | Severity | Fix |
|---------|----------|-----|
| `aggregate_spectral_df` had no `cwsi_col` parameter — couldn't pass explicit CWSI from TB | Medium | Added `cwsi_col: Optional[str] = None` parameter |
| CWSI cascade (explicit → delta-T → VPD → missing) matches `data_schema.py` proxy logic | — | Consistent |
| Physical bounds hardcoded (not in config) | Low | Acceptable; these are physical constants, not tunable params |
| `_clip_or_flag` only checks `isinstance(float)` for NaN | Low | Sensor values are always float; safe |

**Verdict:** Good stateless design. CWSI cascade is well-ordered. Quality flags provide auditability.

---

## 23. scripts/import_layout.py (created 2026-03-10)

| Finding | Severity | Fix |
|---------|----------|-----|
| Row azimuth 315.0° duplicated from ShadowModel default | Low | Acceptable; layout is a snapshot, not a live reference |
| `--print` shadows Python builtin | Low | argparse convention; fine |
| Crop sensor positions (west/east, bottom/upper) parsed from label strings | Low | Works for current 7 Crop devices; fragile if naming changes |

**Verdict:** Useful spatial registration script. Generates a clean JSON for dashboard map view and future ShadowModel integration.

---

## 24. src/models/canopy_photosynthesis.py (modified 2026-03-10)

| Finding | Severity | Fix |
|---------|----------|-----|
| `from config.settings import FRUITING_ZONE_INDEX` was inside method body | Medium | Moved to module-level import |
| `compute_timeseries` doesn't include new fruiting/top fields | Low | Acceptable; it's a batch summary for historical analysis |

**Verdict:** Clean addition. New fields (`fruiting_zone_A`, `fruiting_zone_par`, `top_canopy_A`, `top_canopy_par`) integrate well with existing return dict.