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.