Skip to content

Commit 94df650

Browse files
committed
fix(magnetic-adviser): preserve ranking spread for single-filter scenarios
Two related fixes for the El Choker catalog adviser displaying score=100 on every returned magnetic when only one filter weight is active. 1. MagneticFilterImpedance dropped the dead-band clamp that collapsed every in-band part to penalty=0. With identical raw scorings, normalize_scoring's min==max branch returned 1.0 for every part, wiping out the ranking. The dead band is meaningless for ranking anyway because min-max normalization rescales to [0,1] and discards any constant offset. The score is now the raw mean log10(Zact/Zreq), which is monotonic and produces visible spread. 2. The catalogue overload of get_advised_magnetic now uses a stable tiebreaker (lexicographic reference) when totalScoring values are equal. std::sort on its own is unstable; equal-score blocks were reordered between runs depending on input vector order, breaking ranking determinism in the UI and tests. Adds Test_CatalogueAdviser_Single_Filter_Ranking_Spread regression test (smoke) which mutates a Web_1 catalogue Mas into 4 distinct turn counts, runs the adviser with a single strict Impedance filter, and asserts (a) underlying impedances differ, (b) >= 2 distinct scores, (c) sort is monotonic.
1 parent b508c94 commit 94df650

3 files changed

Lines changed: 109 additions & 21 deletions

File tree

src/advisers/MagneticAdviser.cpp

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -371,9 +371,17 @@ std::vector<std::pair<Mas, double>> MagneticAdviser::get_advised_magnetic(Inputs
371371
logEntry("Found " + std::to_string(masData.size()) + " magnetics", "MagneticAdviser", 2);
372372
auto masMagneticsWithScoring = score_magnetics(masData, filterFlow);
373373

374-
sort(masMagneticsWithScoring.begin(), masMagneticsWithScoring.end(), [](std::pair<Mas, double>& b1, std::pair<Mas, double>& b2) {
375-
return b1.second > b2.second;
376-
});
374+
sort(masMagneticsWithScoring.begin(), masMagneticsWithScoring.end(), [](std::pair<Mas, double>& b1, std::pair<Mas, double>& b2) {
375+
if (b1.second != b2.second) {
376+
return b1.second > b2.second;
377+
}
378+
// Deterministic tiebreaker: lexicographic reference. Without this,
379+
// parts with equal totalScoring (common when one filter dominates
380+
// and other filter contributions saturate) get reordered between
381+
// runs by std::sort's unstable behaviour, breaking ranking
382+
// determinism in the UI and tests.
383+
return b1.first.get_mutable_magnetic().get_reference() < b2.first.get_mutable_magnetic().get_reference();
384+
});
377385

378386
if (masMagneticsWithScoring.size() > maximumNumberResults) {
379387
masMagneticsWithScoring = std::vector<std::pair<Mas, double>>(masMagneticsWithScoring.begin(), masMagneticsWithScoring.end() - (masMagneticsWithScoring.size() - maximumNumberResults));

src/advisers/MagneticFilter.cpp

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1920,26 +1920,23 @@ std::pair<bool, double> MagneticFilterImpedance::evaluate_magnetic(Magnetic* mag
19201920
bool valid = true;
19211921
double scoring = 0;
19221922

1923-
// Impedance scoring: dimensionless log-ratio with industry-standard dead band.
1923+
// Impedance scoring: dimensionless log-ratio.
19241924
//
19251925
// For a "minimum impedance" requirement the part is invalid if Zact < Zreq at any
1926-
// frequency. Among valid parts we want to:
1927-
// - reward parts that comfortably meet the requirement,
1928-
// - not over-reward parts that grossly over-dimension (cost / size penalty),
1929-
// - leave a tolerance band that absorbs typical CMC manufacturing tolerance
1930-
// (~30%) plus a small design margin, so a "well-sized" part is not penalized.
1926+
// frequency. Among valid parts we want to reward parts close to the requirement
1927+
// (smallest over-dimensioning) and penalize gross over-dimensioning (cost/size).
19311928
//
1932-
// Per-frequency penalty:
1933-
// dev_i = log10(max(Zact_i / Zreq_i, 1)) // 0 when under-spec (caught by validity)
1934-
// pen_i = max(0, dev_i - log10(1 + DEAD_BAND))
1929+
// Per-frequency:
1930+
// dev_i = log10(max(Zact_i / Zreq_i, 1)) // 0 when at-spec (under-spec → invalid)
19351931
// Filter score = mean over frequency points (then min-max normalized + inverted
1936-
// by normalize_scoring downstream).
1932+
// downstream so smallest dev becomes the top score).
19371933
//
1938-
// Dead band: 50% (factor 1.5) - covers typical ±25..30% impedance tolerance of
1939-
// common-mode chokes (Würth WE-CMB, TDK ACT, Murata DLW, EPCOS B82xxx) plus
1940-
// ~20% engineering headroom for temperature drift / aging. Tunable below.
1941-
constexpr double IMPEDANCE_DEAD_BAND = 0.50;
1942-
const double deadBandLog = std::log10(1.0 + IMPEDANCE_DEAD_BAND);
1934+
// No dead band on the raw score: normalize_scoring rescales to [0,1] so any
1935+
// constant offset is wiped out, and a hard zero-clamp collapses all in-band parts
1936+
// to the same value -- which then triggers the min==max degenerate branch in
1937+
// normalize_scoring and returns 1.0 for every part (the "all show 100" bug).
1938+
// Keeping the raw monotonic dev preserves ranking spread when this is the only
1939+
// active filter.
19431940

19441941
if (inputs->get_design_requirements().get_minimum_impedance()) {
19451942
auto impedanceRequirement = inputs->get_design_requirements().get_minimum_impedance().value();
@@ -1953,11 +1950,10 @@ std::pair<bool, double> MagneticFilterImpedance::evaluate_magnetic(Magnetic* mag
19531950
}
19541951

19551952
// Ratio is clamped to >= 1: under-spec parts are already invalidated above,
1956-
// so we only score over-dimensioning above the dead band.
1953+
// so we only score over-dimensioning.
19571954
double ratio = (zReq > 0) ? std::max(zAct / zReq, 1.0) : 1.0;
19581955
double dev = std::log10(ratio);
1959-
double penalty = std::max(0.0, dev - deadBandLog);
1960-
scoring += penalty;
1956+
scoring += dev;
19611957
}
19621958
scoring /= impedanceRequirement.size();
19631959
}

tests/TestMagneticAdviser.cpp

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2608,6 +2608,90 @@ namespace {
26082608
REQUIRE(masMagnetics.size() > 0);
26092609
}
26102610

2611+
TEST_CASE("Test_CatalogueAdviser_Single_Filter_Ranking_Spread", "[adviser][magnetic-adviser][catalogue][bug][smoke-test]") {
2612+
// Regression test for "all parts show score 100" bug.
2613+
//
2614+
// When the user enables a single filter (e.g. only Impedance at weight=1,
2615+
// strictlyRequired=true), every returned magnetic in the El Choker UI
2616+
// displayed the same scoring value (1.0 → 100). Root cause: the Impedance
2617+
// filter clamped every in-band part to penalty=0, so all per-part scorings
2618+
// were identical (0). normalize_scoring then hit its min==max degenerate
2619+
// branch and returned 1.0 for every part, collapsing the ranking.
2620+
//
2621+
// This test asserts that with a single strict Impedance filter and a catalogue
2622+
// of magnetics with measurably different impedances, the per-part scorings
2623+
// span at least 2 distinct values — proving the ranking is monotonic.
2624+
2625+
auto json_path = OpenMagneticsTesting::get_test_data_path(std::source_location::current(), "test_catalogueadviser_web_1_2218.json");
2626+
std::ifstream json_file(json_path);
2627+
json catalogueJson = json::parse(json_file);
2628+
2629+
std::vector<OpenMagnetics::Mas> catalogue;
2630+
from_json(catalogueJson, catalogue);
2631+
REQUIRE(catalogue.size() >= 1);
2632+
2633+
// The on-disk fixture is a family of identical magnetics. To get a
2634+
// ranking-spread test we mutate copies of the first Mas with different
2635+
// turn counts (impedance scales monotonically with N for an ungapped
2636+
// ferrite core), producing measurably distinct |Z| at the test frequency.
2637+
OpenMagnetics::Mas templateMas = catalogue[0];
2638+
catalogue.clear();
2639+
for (uint64_t turns : {120UL, 180UL, 240UL, 300UL}) {
2640+
OpenMagnetics::Mas copy = templateMas;
2641+
copy.get_mutable_magnetic().get_mutable_coil().get_mutable_functional_description()[0].set_number_turns(turns);
2642+
// Force a unique reference so add_scoring/get_scorings does not collide.
2643+
MagneticManufacturerInfo info;
2644+
info.set_reference("turns_" + std::to_string(turns));
2645+
copy.get_mutable_magnetic().set_manufacturer_info(info);
2646+
catalogue.push_back(copy);
2647+
}
2648+
REQUIRE(catalogue.size() >= 2);
2649+
2650+
// Inject a minimum-impedance requirement that all magnetics in this catalogue
2651+
// can comfortably meet (so the strict filter does not reject them, and we
2652+
// can observe the ranking spread among the surviving parts).
2653+
ImpedancePoint impedancePoint;
2654+
impedancePoint.set_magnitude(1.0); // 1 Ω at 150 kHz — trivially met by any CMC
2655+
ImpedanceAtFrequency impedanceAtFrequency;
2656+
impedanceAtFrequency.set_frequency(150000);
2657+
impedanceAtFrequency.set_impedance(impedancePoint);
2658+
std::vector<ImpedanceAtFrequency> minimumImpedance{impedanceAtFrequency};
2659+
for (auto& mas : catalogue) {
2660+
mas.get_mutable_inputs().get_mutable_design_requirements().set_minimum_impedance(minimumImpedance);
2661+
}
2662+
2663+
// Single filter, strict, full weight — exactly the El Choker UI scenario when
2664+
// the user sets only the Impedance slider to 100.
2665+
json filterFlowJson = json::parse(R"([{"filter": "Impedance", "invert": true, "log": false, "strictlyRequired": true, "weight": 1 }])");
2666+
std::vector<OpenMagnetics::MagneticFilterOperation> filterFlow = filterFlowJson.get<std::vector<OpenMagnetics::MagneticFilterOperation>>();
2667+
2668+
MagneticAdviser magneticAdviser(false);
2669+
auto masMagnetics = magneticAdviser.get_advised_magnetic(catalogue, filterFlow, catalogue.size());
2670+
REQUIRE(masMagnetics.size() >= 2);
2671+
2672+
// Sanity-check the underlying raw impedances actually differ across the
2673+
// catalogue. If they don't, the test data is unsuitable and we should
2674+
// fail loudly rather than silently passing on identical scores.
2675+
std::set<double> distinctImpedances;
2676+
for (auto& [mas, _] : masMagnetics) {
2677+
auto z = abs(Impedance().calculate_impedance(mas.get_mutable_magnetic(), 150000));
2678+
distinctImpedances.insert(std::round(z * 1e6) / 1e6);
2679+
}
2680+
REQUIRE(distinctImpedances.size() >= 2);
2681+
2682+
// The actual regression assertion: scores must span at least 2 distinct
2683+
// values. Pre-fix this collapsed to {1.0} for every part.
2684+
std::set<double> distinctScores;
2685+
for (auto& [mas, scoring] : masMagnetics) {
2686+
distinctScores.insert(std::round(scoring * 1e6) / 1e6);
2687+
}
2688+
REQUIRE(distinctScores.size() >= 2);
2689+
2690+
// Adviser sorts highest-first; with invert=true on Impedance, the smallest
2691+
// over-dimensioning (closest to requirement) should be the top score.
2692+
REQUIRE(masMagnetics.front().second > masMagnetics.back().second);
2693+
}
2694+
26112695
TEST_CASE("Test_CatalogueAdviser_Web_4", "[adviser][magnetic-adviser][catalogue][bug]") {
26122696
SKIP("Test needs investigation");
26132697

0 commit comments

Comments
 (0)