From 70dba851a8ffa5f3e2611b2982983206e5068f69 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Wed, 10 Jun 2026 14:47:14 -0700 Subject: [PATCH] introduce concept of "bucket" for symbols to provide user-configurable sector dominance calcuation capability --- src/analytics/analysis.zig | 119 +++++++++++--------- src/analytics/observations.zig | 72 ++++++++++-- src/commands/analysis.zig | 8 -- src/commands/review.zig | 10 +- src/models/classification.zig | 197 ++++++++++++++++++++++++++++++++- src/tui/analysis_tab.zig | 31 +++--- src/tui/review_tab.zig | 40 +++---- src/views/review.zig | 109 ++++++++++-------- 8 files changed, 428 insertions(+), 158 deletions(-) diff --git a/src/analytics/analysis.zig b/src/analytics/analysis.zig index eacf217..8a0be13 100644 --- a/src/analytics/analysis.zig +++ b/src/analytics/analysis.zig @@ -242,9 +242,13 @@ pub const AnalysisResult = struct { /// before aggregation. The right field for portfolio-level /// debt-to-equity analysis. asset_category: []BreakdownItem, - /// Breakdown by asset class (US Large Cap, Bonds, Cash & CDs, etc.) - asset_class: []BreakdownItem, - /// Breakdown by sector (Technology, Healthcare, etc.) -- equities only + /// Breakdown by sector bucket (Technology, US Healthcare ETF, + /// US Large Cap, etc.). Aggregates by `entry.bucket` — + /// pre-filled by parseClassificationFile via `deriveBucket`, + /// or curated by the user. Replaces the historical separate + /// "Asset Class" + "Sector" breakdowns: the bucket is a + /// single semantically-meaningful label that combines what + /// each was trying to express. sector: []BreakdownItem, /// Breakdown by geographic region (US, International, etc.) geo: []BreakdownItem, @@ -259,7 +263,6 @@ pub const AnalysisResult = struct { pub fn deinit(self: *AnalysisResult, allocator: std.mem.Allocator) void { allocator.free(self.asset_category); - allocator.free(self.asset_class); allocator.free(self.sector); allocator.free(self.geo); allocator.free(self.account); @@ -285,11 +288,10 @@ pub const Section = struct { /// adding/reordering a section is a one-place edit. Order is /// from coarsest (Asset Category, 4 buckets) to finest /// (per-account / per-tax-type). -pub fn breakdownSections(r: *const AnalysisResult) [6]Section { +pub fn breakdownSections(r: *const AnalysisResult) [5]Section { return .{ .{ .items = r.asset_category, .title = "Asset Category" }, - .{ .items = r.asset_class, .title = "Asset Class" }, - .{ .items = r.sector, .title = "Sector (Equities)" }, + .{ .items = r.sector, .title = "Sector" }, .{ .items = r.geo, .title = "Geographic" }, .{ .items = r.account, .title = "By Account" }, .{ .items = r.tax_type, .title = "By Tax Type" }, @@ -595,9 +597,17 @@ pub fn analyzePortfolio( account_map: ?AccountMap, as_of: Date, ) !AnalysisResult { - // Accumulators: label -> dollar amount - var ac_map = std.StringHashMap(f64).init(allocator); - defer ac_map.deinit(); + // Accumulators: label -> dollar amount. + // + // sector_map and asset_cat_map are both keyed by the + // `bucket` field on ClassificationEntry (pre-filled by + // parseClassificationFile via deriveBucket). Buckets are + // either user-curated, GICS-like sectors, or composite + // "{geo} {asset_class}" labels — meaningful units for + // concentration rollup. The raw `entry.sector` is no + // longer used for either map: NPORT-P fund-decomp + // categories ("Equity / Corporate") would lump genuinely + // different funds together. var sector_map = std.StringHashMap(f64).init(allocator); defer sector_map.deinit(); // 4-bucket coarse breakdown (Equity/Fixed Income/Cash/Other). @@ -614,7 +624,7 @@ pub fn analyzePortfolio( var unclassified_list = std.ArrayList([]const u8).empty; errdefer unclassified_list.deinit(allocator); - // Process each equity allocation (for asset class, sector, geo, unclassified) + // Process each equity allocation (for sector, geo, unclassified) for (allocations) |alloc| { const mv = alloc.market_value; if (mv <= 0) continue; @@ -630,24 +640,34 @@ pub fn analyzePortfolio( const frac = entry.pct / 100.0; const portion = mv * frac; - if (entry.asset_class) |ac| { - const prev = ac_map.get(ac) orelse 0; - try ac_map.put(ac, prev + portion); + // Sector breakdown: roll up by bucket (the + // pre-filled deriveBucket result on the entry). + if (entry.bucket) |b| { + const prev = sector_map.get(b) orelse 0; + try sector_map.put(b, prev + portion); } - // Asset-category bucket: prefer `sector` (richer - // signal). Fall back to `asset_class` for legacy - // hand-written entries that didn't include a - // sector. Counted exactly once per entry. + // Asset Category 4-bucket coarse breakdown + // (Equity / Fixed Income / Cash / Other) keeps + // using the raw `entry.sector` as input. Reasons: + // 1. `bucketSector` recognizes the NPORT-P + // prefixes ("Equity / *", "Debt / *", etc.) + // directly. The user-facing Sector breakdown + // bucket might be "US ETF" (a composite that + // doesn't carry the asset-type signal), + // but the underlying sector still does. + // 2. The Asset Category breakdown is the + // coarse "what's exposed to equity drawdowns?" + // view — invariant to the user's bucket + // curation, since it's a fundamental property + // of the holding. if (entry.sector) |s| { - const prev = sector_map.get(s) orelse 0; - try sector_map.put(s, prev + portion); - const bucket = bucketSector(s); - const bprev = asset_cat_map.get(bucket) orelse 0; - try asset_cat_map.put(bucket, bprev + portion); + const cat = bucketSector(s); + const cprev = asset_cat_map.get(cat) orelse 0; + try asset_cat_map.put(cat, cprev + portion); } else if (entry.asset_class) |ac| { - const bucket = bucketAssetClass(ac); - const bprev = asset_cat_map.get(bucket) orelse 0; - try asset_cat_map.put(bucket, bprev + portion); + const cat = bucketAssetClass(ac); + const cprev = asset_cat_map.get(cat) orelse 0; + try asset_cat_map.put(cat, cprev + portion); } if (entry.geo) |g| { const prev = geo_map.get(g) orelse 0; @@ -698,26 +718,34 @@ pub fn analyzePortfolio( try acct_map.put(acct, prev + value); } - // Add non-stock asset classes (combine Cash + CDs) + // Add non-stock holdings (cash, CDs, options) into the + // coarse asset_category breakdown. They have no entry in + // the classification map (it's keyed by ticker), so we + // route them to coarse buckets directly. const cash_total = portfolio.totalCash(as_of); const cd_total = portfolio.totalCdFaceValue(as_of); const cash_cd_total = cash_total + cd_total; if (cash_cd_total > 0) { - const prev = ac_map.get("Cash & CDs") orelse 0; - try ac_map.put("Cash & CDs", prev + cash_cd_total); const gprev = geo_map.get("US") orelse 0; try geo_map.put("US", gprev + cash_cd_total); // Literal cash and CDs roll into the coarse Cash bucket. const bprev = asset_cat_map.get(bucket_cash) orelse 0; try asset_cat_map.put(bucket_cash, bprev + cash_cd_total); + // Also surface in the Sector breakdown as "Cash & CDs" + // so users with significant cash positions see the + // line. Without this, the Sector breakdown would + // silently omit cash entirely. + const sprev = sector_map.get("Cash & CDs") orelse 0; + try sector_map.put("Cash & CDs", sprev + cash_cd_total); } const opt_total = portfolio.totalOptionCost(as_of); if (opt_total > 0) { - const prev = ac_map.get("Options") orelse 0; - try ac_map.put("Options", prev + opt_total); // Options are derivatives; coarse bucket is Other. const bprev = asset_cat_map.get(bucket_other) orelse 0; try asset_cat_map.put(bucket_other, bprev + opt_total); + // Surface in Sector breakdown too. + const sprev = sector_map.get("Options") orelse 0; + try sector_map.put("Options", sprev + opt_total); } // Tax type breakdown: map each account's total to its tax type @@ -735,7 +763,6 @@ pub fn analyzePortfolio( return .{ .asset_category = try mapToSortedBreakdown(allocator, asset_cat_map, total), - .asset_class = try mapToSortedBreakdown(allocator, ac_map, total), .sector = try mapToSortedBreakdown(allocator, sector_map, total), .geo = try mapToSortedBreakdown(allocator, geo_map, total), .account = try mapToSortedBreakdown(allocator, acct_map, total), @@ -1672,16 +1699,14 @@ test "bucketAssetClass: returns same pointer for same bucket (static-string prop // ── breakdownSections ───────────────────────────────────────── -test "breakdownSections: returns 6 sections" { +test "breakdownSections: returns 5 sections" { var ac_cat = [_]BreakdownItem{}; - var ac = [_]BreakdownItem{}; var sec = [_]BreakdownItem{}; var geo = [_]BreakdownItem{}; var acct = [_]BreakdownItem{}; var tax = [_]BreakdownItem{}; const result = AnalysisResult{ .asset_category = &ac_cat, - .asset_class = &ac, .sector = &sec, .geo = &geo, .account = &acct, @@ -1690,19 +1715,17 @@ test "breakdownSections: returns 6 sections" { .total_value = 0, }; const sections = breakdownSections(&result); - try std.testing.expectEqual(@as(usize, 6), sections.len); + try std.testing.expectEqual(@as(usize, 5), sections.len); } test "breakdownSections: titles in expected order, no leading whitespace, unique" { var ac_cat = [_]BreakdownItem{}; - var ac = [_]BreakdownItem{}; var sec = [_]BreakdownItem{}; var geo = [_]BreakdownItem{}; var acct = [_]BreakdownItem{}; var tax = [_]BreakdownItem{}; const result = AnalysisResult{ .asset_category = &ac_cat, - .asset_class = &ac, .sector = &sec, .geo = &geo, .account = &acct, @@ -1714,8 +1737,7 @@ test "breakdownSections: titles in expected order, no leading whitespace, unique const expected = [_][]const u8{ "Asset Category", - "Asset Class", - "Sector (Equities)", + "Sector", "Geographic", "By Account", "By Tax Type", @@ -1743,16 +1765,14 @@ test "breakdownSections: items.ptr points to AnalysisResult fields" { var ac_cat = [_]BreakdownItem{ .{ .label = "Equity", .weight = 1.0, .value = 100.0 }, }; - var ac = [_]BreakdownItem{ - .{ .label = "US Large Cap", .weight = 0.5, .value = 50.0 }, + var sec = [_]BreakdownItem{ + .{ .label = "Technology", .weight = 0.5, .value = 50.0 }, }; - var sec = [_]BreakdownItem{}; var geo = [_]BreakdownItem{}; var acct = [_]BreakdownItem{}; var tax = [_]BreakdownItem{}; const result = AnalysisResult{ .asset_category = &ac_cat, - .asset_class = &ac, .sector = &sec, .geo = &geo, .account = &acct, @@ -1763,23 +1783,20 @@ test "breakdownSections: items.ptr points to AnalysisResult fields" { const sections = breakdownSections(&result); try std.testing.expectEqual(result.asset_category.ptr, sections[0].items.ptr); - try std.testing.expectEqual(result.asset_class.ptr, sections[1].items.ptr); - try std.testing.expectEqual(result.sector.ptr, sections[2].items.ptr); - try std.testing.expectEqual(result.geo.ptr, sections[3].items.ptr); - try std.testing.expectEqual(result.account.ptr, sections[4].items.ptr); - try std.testing.expectEqual(result.tax_type.ptr, sections[5].items.ptr); + try std.testing.expectEqual(result.sector.ptr, sections[1].items.ptr); + try std.testing.expectEqual(result.geo.ptr, sections[2].items.ptr); + try std.testing.expectEqual(result.account.ptr, sections[3].items.ptr); + try std.testing.expectEqual(result.tax_type.ptr, sections[4].items.ptr); } test "breakdownSections: Asset Category is first (coarse-to-fine ordering)" { var ac_cat = [_]BreakdownItem{}; - var ac = [_]BreakdownItem{}; var sec = [_]BreakdownItem{}; var geo = [_]BreakdownItem{}; var acct = [_]BreakdownItem{}; var tax = [_]BreakdownItem{}; const result = AnalysisResult{ .asset_category = &ac_cat, - .asset_class = &ac, .sector = &sec, .geo = &geo, .account = &acct, diff --git a/src/analytics/observations.zig b/src/analytics/observations.zig index a223506..8a4b6d0 100644 --- a/src/analytics/observations.zig +++ b/src/analytics/observations.zig @@ -391,8 +391,8 @@ fn checkSectorConcentration(ctx: CheckCtx) CheckResult { defer sector_weights.deinit(); for (ctx.rows) |row| { - const existing = sector_weights.get(row.sector_mid) orelse 0; - sector_weights.put(row.sector_mid, existing + row.weight) catch return errResult(ctx.allocator, "alloc failed"); + const existing = sector_weights.get(row.bucket) orelse 0; + sector_weights.put(row.bucket, existing + row.weight) catch return errResult(ctx.allocator, "alloc failed"); } const m: f64 = @floatFromInt(sector_weights.count()); @@ -471,14 +471,24 @@ fn checkSectorDominance(ctx: CheckCtx) CheckResult { var has_flag = false; var has_warn = false; - // O(n²) walk over same-sector pairs. With n typically <50, - // this should be 2500 comparisons of f64s + // O(n²) walk over same-bucket pairs. With n typically <50, + // this should be 2500 comparisons of f64s. + // + // Skip pairs whose bucket contains '/'. Those are the + // NPORT-P fund-decomp categories ("Equity / Corporate", + // "Debt / Corporate", etc.) — meaningless for dominance + // because they lump together genuinely different funds + // (SPY, FRDM, HFXI, VTTHX all sit in "Equity / Corporate"). + // The composite-fallback buckets ("US ETF", "International + // Developed Fund") and user-curated buckets don't contain + // '/' and survive the filter. for (ctx.rows, 0..) |a_row, i| { if (a_row.weight < min_weight) continue; + if (std.mem.indexOfScalar(u8, a_row.bucket, '/') != null) continue; const a_sharpe = a_row.sharpe_3y orelse continue; for (ctx.rows[i + 1 ..]) |b_row| { if (b_row.weight < min_weight) continue; - if (!std.mem.eql(u8, a_row.sector_mid, b_row.sector_mid)) continue; + if (!std.mem.eql(u8, a_row.bucket, b_row.bucket)) continue; const b_sharpe = b_row.sharpe_3y orelse continue; const spread = @abs(a_sharpe - b_sharpe); const sev: ?Severity = if (spread >= dominance_flag_spread) @@ -500,7 +510,7 @@ fn checkSectorDominance(ctx: CheckCtx) CheckResult { winner_sharpe, loser, loser_sharpe, - a_row.sector_mid, + a_row.bucket, spread, }) catch return errResult(ctx.allocator, "alloc failed"); const target = std.fmt.allocPrint(ctx.allocator, "{s},{s}", .{ winner, loser }) catch { @@ -658,7 +668,7 @@ const testing = std.testing; fn makeRow(symbol: []const u8, sector: []const u8, weight: f64) review_view.ReviewRow { return .{ .symbol = symbol, - .sector_mid = sector, + .bucket = sector, .tax_pct = null, .weight = weight, .return_1y = null, @@ -853,6 +863,54 @@ test "checkSectorDominance: tiny holding doesn't trigger pair (min_weight filter try testing.expectEqual(@as(std.meta.Tag(CheckResult), .pass), result); } +test "checkSectorDominance: bucket containing '/' is skipped (NPORT-P mush filter)" { + // Two funds with hugely different Sharpes both bucketed as + // "Equity / Corporate" — the upstream NPORT-P category that + // lumps genuinely-different funds. Filter should suppress + // this pair entirely. + var rows = [_]review_view.ReviewRow{ + makeRowWithVolAndSharpe("FRDM", "Equity / Corporate", 0.30, 0.20, 1.50), + makeRowWithVolAndSharpe("SCHD", "Equity / Corporate", 0.30, 0.13, 0.80), + makeRowWithVolAndSharpe("BND", "Bonds", 0.40, 0.05, 0.40), + }; + const ctx: CheckCtx = .{ + .allocator = testing.allocator, + .rows = &rows, + .totals = emptyTotals(), + }; + const result = checkSectorDominance(ctx); + defer freeResult(testing.allocator, result); + // Spread is 0.70 (flag-worthy) but the '/' filter suppresses it. + try testing.expectEqual(@as(std.meta.Tag(CheckResult), .pass), result); +} + +test "checkSectorDominance: composite-fallback bucket survives the '/' filter" { + // After deriveBucket runs, NPORT-P sectors get composited + // into "International Developed Fund" / similar. Those + // strings DON'T contain '/', so dominance comparisons within + // composite buckets ARE meaningful and should fire. + // + // Weights chosen to clear the min_weight = (1/n) * 0.5 + // threshold (n=3 → min 0.167; both holdings at 0.30). + var rows = [_]review_view.ReviewRow{ + makeRowWithVolAndSharpe("IDMO", "International Developed Fund", 0.30, 0.13, 1.50), + makeRowWithVolAndSharpe("HFXI", "International Developed Fund", 0.30, 0.11, 0.60), + makeRowWithVolAndSharpe("BND", "Bonds", 0.40, 0.05, 0.40), + }; + const ctx: CheckCtx = .{ + .allocator = testing.allocator, + .rows = &rows, + .totals = emptyTotals(), + }; + const result = checkSectorDominance(ctx); + defer freeResult(testing.allocator, result); + // Spread 0.90 = flag. + switch (result) { + .flag => |obs| try testing.expect(obs.len >= 1), + else => return error.TestUnexpectedResult, + } +} + test "checkVolOutlier: holding with 3× portfolio vol flags" { var rows = [_]review_view.ReviewRow{ makeRowWithVolAndSharpe("VTI", "Equity", 0.40, 0.15, 1.0), diff --git a/src/commands/analysis.zig b/src/commands/analysis.zig index 7ff0288..f47e030 100644 --- a/src/commands/analysis.zig +++ b/src/commands/analysis.zig @@ -353,10 +353,6 @@ test "display shows all sections" { .{ .label = "Fixed Income", .weight = 0.15, .value = 15000.0 }, .{ .label = "Cash", .weight = 0.05, .value = 5000.0 }, }; - const asset_class = [_]zfin.analysis.BreakdownItem{ - .{ .label = "US Large Cap", .weight = 0.60, .value = 60000.0 }, - .{ .label = "International", .weight = 0.40, .value = 40000.0 }, - }; const sector = [_]zfin.analysis.BreakdownItem{ .{ .label = "Technology", .weight = 0.35, .value = 35000.0 }, }; @@ -367,7 +363,6 @@ test "display shows all sections" { const unclassified = [_][]const u8{"WEIRD"}; const result: zfin.analysis.AnalysisResult = .{ .asset_category = @constCast(&asset_category), - .asset_class = @constCast(&asset_class), .sector = @constCast(§or), .geo = @constCast(&geo), .account = @constCast(&empty), @@ -383,8 +378,6 @@ test "display shows all sections" { try std.testing.expect(std.mem.indexOf(u8, out, "Fixed Income 15.0%") != null); try std.testing.expect(std.mem.indexOf(u8, out, "Cash 5.0%") != null); try std.testing.expect(std.mem.indexOf(u8, out, "Asset Category") != null); - try std.testing.expect(std.mem.indexOf(u8, out, "Asset Class") != null); - try std.testing.expect(std.mem.indexOf(u8, out, "US Large Cap") != null); try std.testing.expect(std.mem.indexOf(u8, out, "Sector") != null); try std.testing.expect(std.mem.indexOf(u8, out, "Technology") != null); try std.testing.expect(std.mem.indexOf(u8, out, "Geographic") != null); @@ -462,7 +455,6 @@ test "display: includes umbrella section when account_map is provided" { const result: zfin.analysis.AnalysisResult = .{ .asset_category = @constCast(&asset_category), - .asset_class = @constCast(&empty), .sector = @constCast(&empty), .geo = @constCast(&empty), .account = @constCast(&account), diff --git a/src/commands/review.zig b/src/commands/review.zig index a446500..2d0763a 100644 --- a/src/commands/review.zig +++ b/src/commands/review.zig @@ -506,7 +506,7 @@ fn anyReweightFlag(f: portfolio_risk.ReweightFlags) bool { fn renderRow(out: *std.Io.Writer, color: bool, r: review_view.ReviewRow) !void { try out.print(" ", .{}); try out.print("{s:<8}", .{fmt.truncateToCols(r.symbol, col_symbol)}); - try out.print(" {s:<20}", .{fmt.truncateToCols(zfin.analysis.abbreviateSector(r.sector_mid), col_sector)}); + try out.print(" {s:<20}", .{fmt.truncateToCols(zfin.analysis.abbreviateSector(r.bucket), col_sector)}); try out.print(" ", .{}); try renderPctCell(out, color, .normal, r.weight, col_weight, false); try out.print(" ", .{}); @@ -801,7 +801,7 @@ test "renderRow: writes complete row with all fields" { var w: std.Io.Writer = .fixed(&buf); const r: review_view.ReviewRow = .{ .symbol = "VTI", - .sector_mid = "Diversified", + .bucket = "Diversified", .tax_pct = 0.40, .weight = 0.33, .return_1y = 0.15, @@ -830,7 +830,7 @@ test "renderRow: nulls render as em-dashes" { var w: std.Io.Writer = .fixed(&buf); const r: review_view.ReviewRow = .{ .symbol = "NEW", - .sector_mid = "Bonds", + .bucket = "Bonds", .tax_pct = null, .weight = 0.05, .return_1y = null, @@ -909,7 +909,7 @@ test "render: emits header, separator, rows, and totals" { var rows = [_]review_view.ReviewRow{ .{ .symbol = "VTI", - .sector_mid = "Diversified", + .bucket = "Diversified", .tax_pct = 1.0, .weight = 0.6, .return_1y = 0.15, @@ -924,7 +924,7 @@ test "render: emits header, separator, rows, and totals" { }, .{ .symbol = "BND", - .sector_mid = "Bonds", + .bucket = "Bonds", .tax_pct = 0.0, .weight = 0.4, .return_1y = 0.04, diff --git a/src/models/classification.zig b/src/models/classification.zig index fa113df..138d298 100644 --- a/src/models/classification.zig +++ b/src/models/classification.zig @@ -20,6 +20,14 @@ pub const ClassificationEntry = struct { /// have this field. Renderers fall back to `symbol` / /// `display_symbol` when null. name: ?[]const u8 = null, + /// User-curated grouping label that overrides the auto-derived + /// bucket for concentration / dominance checks and the + /// analysis tab's Sector breakdown. Use this when the upstream + /// `sector` field is the NPORT-P "Equity / Corporate" mush + /// that doesn't actually distinguish your holdings (e.g. SPY + /// vs FRDM vs HFXI all tagged the same way). When null, + /// `deriveBucket` falls back to a sensible default. + bucket: ?[]const u8 = null, /// Sector (e.g., "Technology", "Healthcare", "Financials") sector: ?[]const u8 = null, /// Geographic region (e.g., "US", "International Developed", "Emerging Markets") @@ -39,6 +47,7 @@ pub const ClassificationMap = struct { for (self.entries) |e| { self.allocator.free(e.symbol); if (e.name) |n| self.allocator.free(n); + if (e.bucket) |b| self.allocator.free(b); if (e.sector) |s| self.allocator.free(s); if (e.geo) |g| self.allocator.free(g); if (e.asset_class) |a| self.allocator.free(a); @@ -48,7 +57,7 @@ pub const ClassificationMap = struct { }; /// Parse a metadata SRF file into a ClassificationMap. -/// Each record has: symbol::,name::,sector::,geo::,asset_class::,pct:num:

+/// Each record has: symbol::,name::,bucket::,sector::,geo::,asset_class::,pct:num:

/// All fields except symbol are optional. pct defaults to 100. pub fn parseClassificationFile(allocator: std.mem.Allocator, data: []const u8) !ClassificationMap { var entries = std.ArrayList(ClassificationEntry).empty; @@ -56,6 +65,7 @@ pub fn parseClassificationFile(allocator: std.mem.Allocator, data: []const u8) ! for (entries.items) |e| { allocator.free(e.symbol); if (e.name) |n| allocator.free(n); + if (e.bucket) |b| allocator.free(b); if (e.sector) |s| allocator.free(s); if (e.geo) |g| allocator.free(g); if (e.asset_class) |a| allocator.free(a); @@ -69,9 +79,18 @@ pub fn parseClassificationFile(allocator: std.mem.Allocator, data: []const u8) ! while (try it.next()) |fields| { const entry = fields.to(ClassificationEntry, .{}) catch continue; + // Pre-fill `bucket` if the user didn't curate one. This + // shifts the cost of `deriveBucket` to parse time and + // makes downstream code free to read `entry.bucket` + // directly without juggling allocator parameters. + const built_bucket: []const u8 = if (entry.bucket) |b| + try allocator.dupe(u8, b) + else + try deriveBucket(entry, allocator); try entries.append(allocator, .{ .symbol = try allocator.dupe(u8, entry.symbol), .name = if (entry.name) |n| try allocator.dupe(u8, n) else null, + .bucket = built_bucket, .sector = if (entry.sector) |s| try allocator.dupe(u8, s) else null, .geo = if (entry.geo) |g| try allocator.dupe(u8, g) else null, .asset_class = if (entry.asset_class) |a| try allocator.dupe(u8, a) else null, @@ -85,6 +104,48 @@ pub fn parseClassificationFile(allocator: std.mem.Allocator, data: []const u8) ! }; } +/// Resolve a classification entry to its display bucket. Used by +/// the review tab's Sector column, by `analyzePortfolio`'s sector +/// rollup, and by the observation engine's concentration / +/// dominance checks. +/// +/// Four-tier fallback (caller owns the returned slice; allocated +/// via `allocator`): +/// 1. `entry.bucket` if set — user-curated, always wins. +/// 2. `entry.sector` if set AND doesn't contain '/' — GICS-style +/// sector ("Technology", "Healthcare"). The '/' rules out +/// NPORT-P fund-decomp categories ("Equity / Corporate") +/// that are noise rather than meaningful sectors. +/// 3. Composite " " if both are set. For +/// funds without a curated bucket, this gives a meaningful +/// grouping like "International Developed Fund" or "US ETF". +/// 4. Literal "Unclassified". +pub fn deriveBucket(entry: ClassificationEntry, allocator: std.mem.Allocator) ![]const u8 { + if (entry.bucket) |b| return try allocator.dupe(u8, b); + if (entry.sector) |s| { + if (std.mem.indexOfScalar(u8, s, '/') == null) return try allocator.dupe(u8, s); + } + if (entry.geo != null and entry.asset_class != null) { + const g = entry.geo.?; + const ac = entry.asset_class.?; + // Avoid duplicate-geo composites like "US US Large Cap". + // If the asset_class starts with the geo prefix (followed + // by a space or end-of-string), use it alone. Same for + // common geographic-noun asset classes that already imply + // their region ("International Developed", "Emerging + // Markets") — these don't need a geo prefix. + const ac_starts_with_geo = std.mem.startsWith(u8, ac, g) and + (ac.len == g.len or ac[g.len] == ' '); + const ac_has_implicit_geo = std.mem.startsWith(u8, ac, "International") or + std.mem.startsWith(u8, ac, "Emerging"); + if (ac_starts_with_geo or ac_has_implicit_geo) { + return try allocator.dupe(u8, ac); + } + return try std.fmt.allocPrint(allocator, "{s} {s}", .{ g, ac }); + } + return try allocator.dupe(u8, "Unclassified"); +} + test "parse classification file" { const data = \\#!srfv1 @@ -128,9 +189,143 @@ test "parse classification file: missing name field stays null (backwards compat try std.testing.expectEqual(@as(usize, 1), cm.entries.len); try std.testing.expectEqualStrings("AMZN", cm.entries[0].symbol); try std.testing.expect(cm.entries[0].name == null); + // `bucket` is pre-filled by the parser via deriveBucket. For + // a GICS-style sector ("Technology"), it equals the sector. + try std.testing.expectEqualStrings("Technology", cm.entries[0].bucket.?); try std.testing.expectEqualStrings("Technology", cm.entries[0].sector.?); } +test "parse classification file: bucket round-trips" { + const data = + \\#!srfv1 + \\symbol::SPY,name::SPDR S&P 500 ETF Trust,bucket::US Large Cap,sector::Equity / Corporate,geo::US,asset_class::ETF + ; + const allocator = std.testing.allocator; + var cm = try parseClassificationFile(allocator, data); + defer cm.deinit(); + + try std.testing.expectEqual(@as(usize, 1), cm.entries.len); + try std.testing.expectEqualStrings("SPY", cm.entries[0].symbol); + try std.testing.expectEqualStrings("US Large Cap", cm.entries[0].bucket.?); + try std.testing.expectEqualStrings("Equity / Corporate", cm.entries[0].sector.?); +} + +test "deriveBucket: returns user-curated bucket when set" { + const e: ClassificationEntry = .{ + .symbol = "SPY", + .bucket = "US Large Cap", + .sector = "Equity / Corporate", // would otherwise force fallback + .geo = "US", + .asset_class = "ETF", + }; + const out = try deriveBucket(e, std.testing.allocator); + defer std.testing.allocator.free(out); + try std.testing.expectEqualStrings("US Large Cap", out); +} + +test "deriveBucket: returns sector when GICS-like (no '/')" { + const e: ClassificationEntry = .{ + .symbol = "AMZN", + .sector = "Technology", + .geo = "US", + .asset_class = "US Large Cap", + }; + const out = try deriveBucket(e, std.testing.allocator); + defer std.testing.allocator.free(out); + try std.testing.expectEqualStrings("Technology", out); +} + +test "deriveBucket: composite fallback when sector is NPORT-P mush" { + const e: ClassificationEntry = .{ + .symbol = "HFXI", + .sector = "Equity / Corporate", + .geo = "International Developed", + .asset_class = "Fund", + }; + const out = try deriveBucket(e, std.testing.allocator); + defer std.testing.allocator.free(out); + try std.testing.expectEqualStrings("International Developed Fund", out); +} + +test "deriveBucket: returns Unclassified when nothing usable is set" { + const e: ClassificationEntry = .{ + .symbol = "UNK", + }; + const out = try deriveBucket(e, std.testing.allocator); + defer std.testing.allocator.free(out); + try std.testing.expectEqualStrings("Unclassified", out); +} + +test "deriveBucket: NPORT-P sector with no geo/asset_class falls through to Unclassified" { + // Defensive: sector is NPORT-P-style (skipped by the GICS + // filter) AND we don't have both geo and asset_class to + // build a composite. Falls through to Unclassified. + const e: ClassificationEntry = .{ + .symbol = "X", + .sector = "Debt / Corporate", + .geo = "US", + // asset_class missing + }; + const out = try deriveBucket(e, std.testing.allocator); + defer std.testing.allocator.free(out); + try std.testing.expectEqualStrings("Unclassified", out); +} + +test "deriveBucket: composite avoids duplicate geo when asset_class already starts with it" { + // Hand-written entries often have geographically-prefixed + // asset_class values like "US Large Cap" alongside + // geo="US". The naive composite "{geo} {asset_class}" then + // produces "US US Large Cap" which is ugly and clusters + // incorrectly in the breakdown. Detect the duplicate prefix + // and use the asset_class alone. + const e: ClassificationEntry = .{ + .symbol = "VOO", + .geo = "US", + .asset_class = "US Large Cap", + }; + const out = try deriveBucket(e, std.testing.allocator); + defer std.testing.allocator.free(out); + try std.testing.expectEqualStrings("US Large Cap", out); +} + +test "deriveBucket: composite uses asset_class alone for International/Emerging implicit-geo classes" { + // "International Developed" and "Emerging Markets" are + // already geographic; the composite shouldn't re-prepend + // the geo. + const e1: ClassificationEntry = .{ + .symbol = "VEA", + .geo = "International Developed", + .asset_class = "International Developed", + }; + const out1 = try deriveBucket(e1, std.testing.allocator); + defer std.testing.allocator.free(out1); + try std.testing.expectEqualStrings("International Developed", out1); + + const e2: ClassificationEntry = .{ + .symbol = "VWO", + .geo = "Emerging Markets", + .asset_class = "Emerging Markets", + }; + const out2 = try deriveBucket(e2, std.testing.allocator); + defer std.testing.allocator.free(out2); + try std.testing.expectEqualStrings("Emerging Markets", out2); +} + +test "deriveBucket: composite still prepends geo when asset_class is generic (Fund/ETF/Bonds)" { + // The whole point of the composite is to disambiguate + // generic asset_class labels by their geo. Make sure we + // don't accidentally regress on this case while fixing + // the duplicate-prefix one. + const e: ClassificationEntry = .{ + .symbol = "BND", + .geo = "US", + .asset_class = "Fund", + }; + const out = try deriveBucket(e, std.testing.allocator); + defer std.testing.allocator.free(out); + try std.testing.expectEqualStrings("US Fund", out); +} + // ── ClassificationRecord ───────────────────────────────────── // // Distinct from `ClassificationEntry` above: that one represents diff --git a/src/tui/analysis_tab.zig b/src/tui/analysis_tab.zig index 471bebf..109032f 100644 --- a/src/tui/analysis_tab.zig +++ b/src/tui/analysis_tab.zig @@ -274,7 +274,6 @@ pub fn renderAnalysisLines( // gets freed at tab.deinit time. const display_result: zfin.analysis.AnalysisResult = .{ .asset_category = result.asset_category, - .asset_class = result.asset_class, .sector = collapsed_sector, .geo = result.geo, .account = result.account, @@ -292,7 +291,7 @@ pub fn renderAnalysisLines( // section's title string). For the Sector section, // append the current granularity in parens so the user // knows what the `g` hot-key cycled to. - const title_text = if (std.mem.eql(u8, sec.title, "Sector (Equities)")) + const title_text = if (std.mem.eql(u8, sec.title, "Sector")) try std.fmt.allocPrint(arena, " Sector ({s} — press 'm' to cycle)", .{granularityLabel(sector_granularity)}) else try std.fmt.allocPrint(arena, " {s}", .{sec.title}); @@ -449,14 +448,17 @@ test "renderAnalysisLines with data" { const arena = arena_state.allocator(); const th = theme.default_theme; - var asset_class = [_]zfin.analysis.BreakdownItem{ - .{ .label = "US Stock", .weight = 0.60, .value = 120000 }, - .{ .label = "Int'l Stock", .weight = 0.40, .value = 80000 }, + // Use sector breakdown (the main fine-grained slice) as the + // populated section for this test. asset_class is gone — its + // role is subsumed by the bucket-driven `sector` field. + // Use GICS-style labels that survive `midBucket` collapse. + var sector = [_]zfin.analysis.BreakdownItem{ + .{ .label = "Technology", .weight = 0.60, .value = 120000 }, + .{ .label = "Healthcare", .weight = 0.40, .value = 80000 }, }; const result = zfin.analysis.AnalysisResult{ .asset_category = &.{}, - .asset_class = &asset_class, - .sector = &.{}, + .sector = §or, .geo = &.{}, .account = &.{}, .tax_type = &.{}, @@ -464,7 +466,7 @@ test "renderAnalysisLines with data" { .total_value = 200000, }; const lines = try renderAnalysisLines(arena, th, result, 0.80, 0.15, 0.05, 200000, .mid, null); - // Should have header section + asset class items + // Should have header section + sector items try testing.expect(lines.len >= 5); // Find "Portfolio Analysis" header var found_header = false; @@ -475,12 +477,12 @@ test "renderAnalysisLines with data" { } try testing.expect(found_header); try testing.expect(found_cash_in_summary); - // Find asset class data - var found_us = false; + // Find sector breakdown data + var found_tech = false; for (lines) |l| { - if (std.mem.indexOf(u8, l.text, "US Stock") != null) found_us = true; + if (std.mem.indexOf(u8, l.text, "Technology") != null) found_tech = true; } - try testing.expect(found_us); + try testing.expect(found_tech); } test "renderAnalysisLines no data" { @@ -535,7 +537,6 @@ test "renderAnalysisLines: granularity label appears in Sector section title" { }; const result = zfin.analysis.AnalysisResult{ .asset_category = &.{}, - .asset_class = &.{}, .sector = §or, .geo = &.{}, .account = &.{}, @@ -587,7 +588,6 @@ test "renderAnalysisLines: mid granularity collapses Debt rows" { }; const result = zfin.analysis.AnalysisResult{ .asset_category = &.{}, - .asset_class = &.{}, .sector = §or, .geo = &.{}, .account = &.{}, @@ -622,7 +622,6 @@ test "renderAnalysisLines: umbrella section appears at the bottom when account_m }; const result = zfin.analysis.AnalysisResult{ .asset_category = &.{}, - .asset_class = &.{}, .sector = &.{}, .geo = &.{}, .account = &account, @@ -670,7 +669,6 @@ test "renderAnalysisLines: umbrella section absent when account_map is null" { }; const result = zfin.analysis.AnalysisResult{ .asset_category = &.{}, - .asset_class = &.{}, .sector = &.{}, .geo = &.{}, .account = &account, @@ -701,7 +699,6 @@ test "renderAnalysisLines: umbrella respects shielded:bool:false override (DCP c }; const result = zfin.analysis.AnalysisResult{ .asset_category = &.{}, - .asset_class = &.{}, .sector = &.{}, .geo = &.{}, .account = &account, diff --git a/src/tui/review_tab.zig b/src/tui/review_tab.zig index d73896c..ad009d1 100644 --- a/src/tui/review_tab.zig +++ b/src/tui/review_tab.zig @@ -1760,7 +1760,7 @@ fn formatRow( try rb.cellLeft(r.symbol, col_symbol, .normal); try rb.gap(); - try rb.cellLeft(zfin.analysis.abbreviateSector(r.sector_mid), col_sector, .normal); + try rb.cellLeft(zfin.analysis.abbreviateSector(r.bucket), col_sector, .normal); try rb.gap(); try rb.cellRight(fmt.fmtPct(&pct_buf, r.weight, .{}), col_weight, .normal); @@ -1956,7 +1956,7 @@ test "formatRow: produces a styled line containing symbol + sector" { const r: review_view.ReviewRow = .{ .symbol = "VTI", - .sector_mid = "Diversified", + .bucket = "Diversified", .tax_pct = 0.40, .weight = 0.33, .return_1y = 0.15, @@ -1982,7 +1982,7 @@ test "formatRow: nulls render as em-dashes" { const r: review_view.ReviewRow = .{ .symbol = "NEW", - .sector_mid = "Bonds", + .bucket = "Bonds", .tax_pct = null, .weight = 0.05, .return_1y = null, @@ -2006,7 +2006,7 @@ test "formatRow: abbreviates Communication Services" { const r: review_view.ReviewRow = .{ .symbol = "GOOGL", - .sector_mid = "Communication Services", + .bucket = "Communication Services", .tax_pct = null, .weight = 0.05, .return_1y = null, @@ -2031,7 +2031,7 @@ test "formatRow: is_active=true emits leading '* ' marker" { const r: review_view.ReviewRow = .{ .symbol = "VTI", - .sector_mid = "Diversified", + .bucket = "Diversified", .tax_pct = null, .weight = 0.5, .return_1y = null, @@ -2105,7 +2105,7 @@ test "applySort: explicit field replaces default grouping" { var rows = [_]review_view.ReviewRow{ .{ .symbol = "AAPL", - .sector_mid = "Technology", + .bucket = "Technology", .tax_pct = null, .weight = 0.05, .return_1y = null, @@ -2120,7 +2120,7 @@ test "applySort: explicit field replaces default grouping" { }, .{ .symbol = "VTI", - .sector_mid = "Equity / Corporate", + .bucket = "Equity / Corporate", .tax_pct = null, .weight = 0.40, .return_1y = null, @@ -2135,7 +2135,7 @@ test "applySort: explicit field replaces default grouping" { }, .{ .symbol = "MSFT", - .sector_mid = "Technology", + .bucket = "Technology", .tax_pct = null, .weight = 0.15, .return_1y = null, @@ -2175,11 +2175,11 @@ test "applySort: explicit field replaces default grouping" { // Default grouping (sector asc with symbol-asc tiebreaker) → // Equity/Corporate first (only VTI), then Technology with // AAPL before MSFT (alphabetical). - try testing.expectEqualStrings("Equity / Corporate", state.view.?.rows[0].sector_mid); + try testing.expectEqualStrings("Equity / Corporate", state.view.?.rows[0].bucket); try testing.expectEqualStrings("VTI", state.view.?.rows[0].symbol); - try testing.expectEqualStrings("Technology", state.view.?.rows[1].sector_mid); + try testing.expectEqualStrings("Technology", state.view.?.rows[1].bucket); try testing.expectEqualStrings("AAPL", state.view.?.rows[1].symbol); - try testing.expectEqualStrings("Technology", state.view.?.rows[2].sector_mid); + try testing.expectEqualStrings("Technology", state.view.?.rows[2].bucket); try testing.expectEqualStrings("MSFT", state.view.?.rows[2].symbol); // Explicit weight desc @@ -2609,7 +2609,7 @@ test "handleAction: toggle_expand on cursor in holdings is no-op" { // finding to expand. var rows = [_]review_view.ReviewRow{.{ .symbol = "X", - .sector_mid = "S", + .bucket = "S", .tax_pct = 0, .weight = 1.0, .return_1y = null, @@ -2645,7 +2645,7 @@ test "onCursorMove: walks through holdings then wraps into findings" { // 3 holdings + 2 findings = 5 unified rows. var holdings = [_]review_view.ReviewRow{ .{ .symbol = "A", - .sector_mid = "S", + .bucket = "S", .tax_pct = 0, .weight = 0.33, .return_1y = null, @@ -2659,7 +2659,7 @@ test "onCursorMove: walks through holdings then wraps into findings" { .maxdd_5y = null, }, .{ .symbol = "B", - .sector_mid = "S", + .bucket = "S", .tax_pct = 0, .weight = 0.33, .return_1y = null, @@ -2673,7 +2673,7 @@ test "onCursorMove: walks through holdings then wraps into findings" { .maxdd_5y = null, }, .{ .symbol = "C", - .sector_mid = "S", + .bucket = "S", .tax_pct = 0, .weight = 0.34, .return_1y = null, @@ -2741,7 +2741,7 @@ test "onCursorMove: walks through holdings then wraps into findings" { test "onCursorMove: backward from first holding wraps to last finding" { var holdings = [_]review_view.ReviewRow{.{ .symbol = "A", - .sector_mid = "S", + .bucket = "S", .tax_pct = 0, .weight = 1.0, .return_1y = null, @@ -2897,7 +2897,7 @@ test "handleMouse: click on holdings row sets unified cursor" { for (rows, 0..) |*r, i| { r.* = .{ .symbol = "X", - .sector_mid = "S", + .bucket = "S", .tax_pct = 0, .weight = 0.33, .return_1y = null, @@ -3140,7 +3140,7 @@ test "cursorSection: empty when no view loaded" { test "cursorSection: holdings when cursor < holdings_len" { var rows = [_]review_view.ReviewRow{ .{ .symbol = "A", - .sector_mid = "S", + .bucket = "S", .tax_pct = 0, .weight = 1.0, .return_1y = null, @@ -3154,7 +3154,7 @@ test "cursorSection: holdings when cursor < holdings_len" { .maxdd_5y = null, }, .{ .symbol = "B", - .sector_mid = "S", + .bucket = "S", .tax_pct = 0, .weight = 1.0, .return_1y = null, @@ -3216,7 +3216,7 @@ test "unackCurrentFinding: cursor in holdings emits status hint" { // status message and not touch any journal. var rows = [_]review_view.ReviewRow{.{ .symbol = "X", - .sector_mid = "S", + .bucket = "S", .tax_pct = 0, .weight = 1.0, .return_1y = null, diff --git a/src/views/review.zig b/src/views/review.zig index 067c80d..bc71c90 100644 --- a/src/views/review.zig +++ b/src/views/review.zig @@ -80,7 +80,7 @@ pub const ReviewRow = struct { /// Mid-granularity sector label (`analytics/analysis.midBucket` /// applied to the user's `metadata.srf` classification). Falls back /// to "Unclassified" when the symbol has no classification entry. - sector_mid: []const u8, + bucket: []const u8, /// Fraction of the holding's market value in taxable accounts /// (0.0 = fully tax-advantaged, 1.0 = fully taxable). Computed /// from per-lot accounts; null when the AccountMap is missing @@ -207,7 +207,7 @@ pub fn buildReview( else null; - const sector_mid = sectorForSymbol(a.symbol, classifications); + const bucket = bucketForSymbol(a.symbol, classifications); const tax_pct = computeTaxPct(a.symbol, portfolio, account_map, as_of); const tr_returns = computeTrailingReturns(candles, dividends, as_of); @@ -215,7 +215,7 @@ pub fn buildReview( try rows.append(allocator, .{ .symbol = a.display_symbol, - .sector_mid = sector_mid, + .bucket = bucket, .tax_pct = tax_pct, .weight = a.weight, .return_1y = annualizedFromResult(tr_returns.one_year, false), @@ -428,7 +428,7 @@ fn sortLessThan(ctx: SortCtx, a: ReviewRow, b: ReviewRow) bool { fn extractStr(field: SortField, r: ReviewRow) []const u8 { return switch (field) { .symbol => r.symbol, - .sector => r.sector_mid, + .sector => r.bucket, else => unreachable, }; } @@ -473,26 +473,28 @@ fn sortFloatByDir(a: ?f64, b: ?f64, dir: SortDirection) bool { // ── Internal helpers ────────────────────────────────────────── -/// Walk the classification map for a symbol. Returns the mid-granularity -/// sector label (a static literal per `analysis.midBucket`) or -/// "Unclassified" if no entry exists. -fn sectorForSymbol(symbol: []const u8, classifications: classification.ClassificationMap) []const u8 { - // Find the FIRST matching classification entry. Multi-row - // classifications (target-date funds with proportional splits) are - // collapsed to their primary sector here — the review view is - // per-holding, not per-classification-component, so we pick the - // most-weighted entry. - var best_pct: f64 = 0; - var best_sector: ?[]const u8 = null; - for (classifications.entries) |e| { +/// Resolve a symbol's display bucket. Walks the classification +/// map; returns the most-weighted entry's `bucket` field. The +/// `bucket` field is always populated by `parseClassificationFile` +/// (via `deriveBucket` fallback when not user-curated), so the +/// returned slice is always non-empty and borrows from the +/// ClassificationMap's storage. +fn bucketForSymbol( + symbol: []const u8, + classifications: classification.ClassificationMap, +) []const u8 { + var best_pct: f64 = -1; + var best_idx: ?usize = null; + for (classifications.entries, 0..) |e, i| { if (!std.mem.eql(u8, e.symbol, symbol)) continue; - if (e.sector == null) continue; if (e.pct > best_pct) { best_pct = e.pct; - best_sector = e.sector; + best_idx = i; } } - if (best_sector) |s| return analysis.collapseSector(s, .mid); + if (best_idx) |i| { + if (classifications.entries[i].bucket) |b| return b; + } return "Unclassified"; } @@ -679,7 +681,7 @@ const testing = std.testing; fn makeRow(symbol: []const u8, sector: []const u8, weight: f64) ReviewRow { return .{ .symbol = symbol, - .sector_mid = sector, + .bucket = sector, .tax_pct = null, .weight = weight, .return_1y = null, @@ -767,9 +769,9 @@ test "sortRows: by symbol desc is reverse alphabetical (string desc path)" { test "sortRows: by tax_pct asc (covers ascending float comparator path)" { var rows = [_]ReviewRow{ - .{ .symbol = "A", .sector_mid = "x", .tax_pct = 0.8, .weight = 0.1, .return_1y = null, .return_3y = null, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, - .{ .symbol = "B", .sector_mid = "x", .tax_pct = 0.1, .weight = 0.1, .return_1y = null, .return_3y = null, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, - .{ .symbol = "C", .sector_mid = "x", .tax_pct = 0.5, .weight = 0.1, .return_1y = null, .return_3y = null, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, + .{ .symbol = "A", .bucket = "x", .tax_pct = 0.8, .weight = 0.1, .return_1y = null, .return_3y = null, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, + .{ .symbol = "B", .bucket = "x", .tax_pct = 0.1, .weight = 0.1, .return_1y = null, .return_3y = null, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, + .{ .symbol = "C", .bucket = "x", .tax_pct = 0.5, .weight = 0.1, .return_1y = null, .return_3y = null, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, }; sortRows(&rows, .tax_pct, .asc); try testing.expectEqualStrings("B", rows[0].symbol); @@ -793,8 +795,8 @@ test "sortRows: every numeric SortField variant is reachable via extractFloat" { }; inline for (numeric_fields) |field| { var rows = [_]ReviewRow{ - .{ .symbol = "low", .sector_mid = "x", .tax_pct = 0.1, .weight = 0.1, .return_1y = 0.1, .return_3y = 0.1, .return_5y = 0.1, .return_10y = 0.1, .vol_3y = 0.1, .vol_10y = 0.1, .sharpe_3y = 0.1, .sharpe_10y = 0.1, .maxdd_5y = 0.1 }, - .{ .symbol = "high", .sector_mid = "x", .tax_pct = 0.9, .weight = 0.9, .return_1y = 0.9, .return_3y = 0.9, .return_5y = 0.9, .return_10y = 0.9, .vol_3y = 0.9, .vol_10y = 0.9, .sharpe_3y = 0.9, .sharpe_10y = 0.9, .maxdd_5y = 0.9 }, + .{ .symbol = "low", .bucket = "x", .tax_pct = 0.1, .weight = 0.1, .return_1y = 0.1, .return_3y = 0.1, .return_5y = 0.1, .return_10y = 0.1, .vol_3y = 0.1, .vol_10y = 0.1, .sharpe_3y = 0.1, .sharpe_10y = 0.1, .maxdd_5y = 0.1 }, + .{ .symbol = "high", .bucket = "x", .tax_pct = 0.9, .weight = 0.9, .return_1y = 0.9, .return_3y = 0.9, .return_5y = 0.9, .return_10y = 0.9, .vol_3y = 0.9, .vol_10y = 0.9, .sharpe_3y = 0.9, .sharpe_10y = 0.9, .maxdd_5y = 0.9 }, }; sortRows(&rows, field, .desc); try testing.expectEqualStrings("high", rows[0].symbol); @@ -946,23 +948,23 @@ test "sortGroupedByDefault: groups by sector then symbol asc within group" { // Sectors alphabetical: Bonds → Equity / Corporate → Technology. // Within each sector, symbols alphabetical (deterministic, easy // to scan for a specific ticker). - try testing.expectEqualStrings("Bonds", rows[0].sector_mid); + try testing.expectEqualStrings("Bonds", rows[0].bucket); try testing.expectEqualStrings("AGG", rows[0].symbol); - try testing.expectEqualStrings("Bonds", rows[1].sector_mid); + try testing.expectEqualStrings("Bonds", rows[1].bucket); try testing.expectEqualStrings("BND", rows[1].symbol); - try testing.expectEqualStrings("Equity / Corporate", rows[2].sector_mid); + try testing.expectEqualStrings("Equity / Corporate", rows[2].bucket); try testing.expectEqualStrings("VTI", rows[2].symbol); - try testing.expectEqualStrings("Technology", rows[3].sector_mid); + try testing.expectEqualStrings("Technology", rows[3].bucket); try testing.expectEqualStrings("AAPL", rows[3].symbol); - try testing.expectEqualStrings("Technology", rows[4].sector_mid); + try testing.expectEqualStrings("Technology", rows[4].bucket); try testing.expectEqualStrings("MSFT", rows[4].symbol); } test "sortRows: nulls sort to end on both directions" { var rows_desc = [_]ReviewRow{ - .{ .symbol = "A", .sector_mid = "x", .tax_pct = null, .weight = 0.1, .return_1y = null, .return_3y = 0.10, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, - .{ .symbol = "B", .sector_mid = "x", .tax_pct = null, .weight = 0.1, .return_1y = null, .return_3y = 0.20, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, - .{ .symbol = "C", .sector_mid = "x", .tax_pct = null, .weight = 0.1, .return_1y = null, .return_3y = null, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, + .{ .symbol = "A", .bucket = "x", .tax_pct = null, .weight = 0.1, .return_1y = null, .return_3y = 0.10, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, + .{ .symbol = "B", .bucket = "x", .tax_pct = null, .weight = 0.1, .return_1y = null, .return_3y = 0.20, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, + .{ .symbol = "C", .bucket = "x", .tax_pct = null, .weight = 0.1, .return_1y = null, .return_3y = null, .return_5y = null, .return_10y = null, .vol_3y = null, .vol_10y = null, .sharpe_3y = null, .sharpe_10y = null, .maxdd_5y = null }, }; sortRows(&rows_desc, .return_3y, .desc); try testing.expectEqualStrings("B", rows_desc[0].symbol); // 0.20 @@ -988,8 +990,8 @@ test "WeightedAvg: all-null returns null" { test "computeTotals: weighted average returns + synthetic risk pass-through" { const rows = [_]ReviewRow{ - .{ .symbol = "A", .sector_mid = "x", .tax_pct = 1.0, .weight = 0.6, .return_1y = 0.10, .return_3y = 0.10, .return_5y = null, .return_10y = null, .vol_3y = 0.20, .vol_10y = null, .sharpe_3y = 1.0, .sharpe_10y = null, .maxdd_5y = 0.30 }, - .{ .symbol = "B", .sector_mid = "y", .tax_pct = 0.0, .weight = 0.4, .return_1y = 0.20, .return_3y = 0.05, .return_5y = null, .return_10y = null, .vol_3y = 0.10, .vol_10y = null, .sharpe_3y = 0.5, .sharpe_10y = null, .maxdd_5y = 0.10 }, + .{ .symbol = "A", .bucket = "x", .tax_pct = 1.0, .weight = 0.6, .return_1y = 0.10, .return_3y = 0.10, .return_5y = null, .return_10y = null, .vol_3y = 0.20, .vol_10y = null, .sharpe_3y = 1.0, .sharpe_10y = null, .maxdd_5y = 0.30 }, + .{ .symbol = "B", .bucket = "y", .tax_pct = 0.0, .weight = 0.4, .return_1y = 0.20, .return_3y = 0.05, .return_5y = null, .return_10y = null, .vol_3y = 0.10, .vol_10y = null, .sharpe_3y = 0.5, .sharpe_10y = null, .maxdd_5y = 0.10 }, }; const synth: portfolio_risk.SyntheticRisk = .{ .vol_3y = 0.13, @@ -1011,37 +1013,40 @@ test "computeTotals: weighted average returns + synthetic risk pass-through" { try testing.expectApproxEqAbs(@as(f64, 0.18), t.maxdd_5y.?, 0.0001); } -test "sectorForSymbol: returns Unclassified for unknown symbol" { +test "bucketForSymbol: returns Unclassified for unknown symbol" { var entries = [_]classification.ClassificationEntry{}; const cm: classification.ClassificationMap = .{ .entries = entries[0..], .allocator = testing.allocator, }; - try testing.expectEqualStrings("Unclassified", sectorForSymbol("NOPE", cm)); + try testing.expectEqualStrings("Unclassified", bucketForSymbol("NOPE", cm)); } -test "sectorForSymbol: returns mid-bucket for classified symbol" { +test "bucketForSymbol: returns bucket field directly (parser pre-fills it)" { + // bucketForSymbol no longer calls deriveBucket — it just + // reads `entry.bucket` which is pre-populated by + // `parseClassificationFile`. Tests that synthesize entries + // by hand must set `bucket` themselves. var entries = [_]classification.ClassificationEntry{ - .{ .symbol = "AAPL", .sector = "Technology", .pct = 100.0 }, + .{ .symbol = "AAPL", .bucket = "Technology", .sector = "Technology", .pct = 100.0 }, }; const cm: classification.ClassificationMap = .{ .entries = entries[0..], .allocator = testing.allocator, }; - // Technology is a GICS sector; mid-bucket passes it through. - try testing.expectEqualStrings("Technology", sectorForSymbol("AAPL", cm)); + try testing.expectEqualStrings("Technology", bucketForSymbol("AAPL", cm)); } -test "sectorForSymbol: collapses NPORT-P sub-flavors via mid-bucket" { +test "bucketForSymbol: multi-row entry picks most-weighted row's bucket" { var entries = [_]classification.ClassificationEntry{ - .{ .symbol = "BND", .sector = "Debt / US Treasury", .pct = 100.0 }, + .{ .symbol = "X", .bucket = "Minor", .pct = 10.0 }, + .{ .symbol = "X", .bucket = "Major", .pct = 90.0 }, }; const cm: classification.ClassificationMap = .{ .entries = entries[0..], .allocator = testing.allocator, }; - // "Debt / *" maps to "Bonds" at mid granularity. - try testing.expectEqualStrings("Bonds", sectorForSymbol("BND", cm)); + try testing.expectEqualStrings("Major", bucketForSymbol("X", cm)); } test "volIntent: thresholds bucketize correctly" { @@ -1148,8 +1153,12 @@ test "buildReview: end-to-end with testing allocator (leak check)" { defer candle_map.deinit(); var class_entries = [_]classification.ClassificationEntry{ - .{ .symbol = "VTI", .sector = "Equity / Corporate", .pct = 100.0 }, - .{ .symbol = "BND", .sector = "Debt / US Treasury", .pct = 100.0 }, + // bucket is normally pre-filled by parseClassificationFile; + // hand-synthesized entries set it explicitly. For NPORT-P- + // style sectors with geo+asset_class, deriveBucket would + // produce the composite "{geo} {asset_class}". + .{ .symbol = "VTI", .bucket = "US ETF", .sector = "Equity / Corporate", .geo = "US", .asset_class = "ETF", .pct = 100.0 }, + .{ .symbol = "BND", .bucket = "US Fund", .sector = "Debt / US Treasury", .geo = "US", .asset_class = "Fund", .pct = 100.0 }, }; const cm: classification.ClassificationMap = .{ .entries = class_entries[0..], @@ -1184,13 +1193,15 @@ test "buildReview: end-to-end with testing allocator (leak check)" { // Tax%: VTI is in taxable account (1.0), BND in Roth (0.0). // Find each row by symbol since order isn't yet sorted. + // Buckets: both are NPORT-P-style sectors so deriveBucket + // falls through to the (geo, asset_class) composite. for (view.rows) |r| { if (std.mem.eql(u8, r.symbol, "VTI")) { try testing.expectApproxEqAbs(@as(f64, 1.0), r.tax_pct.?, 0.001); - try testing.expectEqualStrings("Equity / Corporate", r.sector_mid); + try testing.expectEqualStrings("US ETF", r.bucket); } else if (std.mem.eql(u8, r.symbol, "BND")) { try testing.expectApproxEqAbs(@as(f64, 0.0), r.tax_pct.?, 0.001); - try testing.expectEqualStrings("Bonds", r.sector_mid); + try testing.expectEqualStrings("US Fund", r.bucket); } }