Conversation
…uator Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1-Bort-1
left a comment
There was a problem hiding this comment.
Independent review (advisory)
Verdict: APPROVE WITH COMMENTS · 2 inline, 0 off the diff
Good
- Matches the card: the error stub for masure_regression is gone, and the new branch in resolve_airfoil follows the neuralfoil branch (writes a polar table and returns "polars"). Checked in the airfoil_io.jl hunk.
- The Float32 cast in scale_input matches sklearn's tree comparison: a Float32 input promoted against a Float64 threshold. The fixture's on-split rows from threshold_rows would fail if the cast were dropped, which matches the card's 31/5 red run.
- Folding everything into the single write_polar(filepath, alpha, cl, cd, cm) keeps behaviour. write_node_rows expects radians, NeuralFoilResult still passes deg2rad.(alpha), and SectionSolution passes sol.alpha unchanged, as it did before.
- Output order is handled consistently: the model order is CD, CL, CM, masure_aero returns (cl, cd, cm), and the parity test compares [cd, cl, cm] with sklearn's columns.
- No new dependency: NPZ was already in Project.toml and is used by neuralfoil.jl. The exporter raises on any unexpected pipeline layout instead of guessing.
- Every new public and private symbol is listed in functions.md, private_functions.md or private_types.md, and the pipeline page documents the parameters and units.
Not good
test/airfoil_aero/test_masure_regression.jl:17— The card says the Julia evaluator matches sklearn to the last bit, but the test only checks atol=1e-12, so a small bit-level drift (e.g. summation order in the mean over trees) would pass unnoticed. Use==to protect the property the PR claims.src/airfoil_aero/masure_regression.jl:62— Re in the cache key is redundant because the path already encodes it. The isfile check and loading logic sit in a do-closure, against §6. A plainhaskey/load/store helper would be shorter and clearer.- The parity test uses
≈ expected atol=1e-12, but the card claims a bit-exact match (max difference 0.0).==would pin the claim the PR is built on. - MASURE_CACHE is keyed on (Float64(Re), abspath(path)), but the path already encodes Re, so the Re in the key is redundant. Also, re-exporting into the same directory in a live session returns the stale model.
- load_masure_model puts the isfile check and loading logic inside a get! do-closure, and ExtraTreesForest defines a local one_based closure. §6 says to keep logic out of nested closures.
- The two-line header comment in masure_regression.jl says again what the docstrings of load_masure_model and predict already say. It fails §7's deletion test.
- resolve_aero_geometry's default alpha_range is -180:180. Extra-Trees silently return a constant leaf outside the trained alpha range, so a masure airfoil without an explicit alpha_range gets a flat, plausible-looking polar with no warning.
- A missing key in the masure info_dict (e.g. the old 'lamba' spelling in a user file) surfaces as a bare KeyError from masure_aero, not an error naming the airfoil id and the expected MASURE_PARAMETERS.
- Private
predictis a very generic name inside AirfoilAero. Something likeforest_predictwould avoid confusion with MLJ/StatsAPIpredictif either ever enters the namespace. - threshold_rows in the exporter reads named_steps["scale"]/["model"]. Those step names exist only in the fixture pipeline; export_model correctly uses positional steps.
- The new prose in airfoil_pipeline.md and CHANGELOG.md is hard-wrapped. §6 says prose is one line per paragraph, though the surrounding existing text is wrapped the same way.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
The parity test compares with == instead of atol=1e-12. resolve_airfoil names the airfoil and the missing keys instead of a bare KeyError. load_masure_model caches on the model path alone, without the do-closure; predict is renamed forest_predict; the exporter's fixture helper reads the pipeline steps positionally, as export_model does. New prose is unwrapped, and the docs say outside the trained alpha range the trees are constant. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Fixed in 7132853: parity checked with ==; cache keyed on path, with no closure in the loader; one_based moved to a named module function; the duplicate header comment deleted; predict renamed to forest_predict; resolve_airfoil now names the airfoil id and missing keys (the test was red before with a KeyError); the exporter's threshold_rows reads steps by position, and the regenerated fixture is byte-identical; the new prose is unwrapped. Not fixed: warning when alpha falls outside the trained range. The .npz has no record of that range, so a warning means changing the export format. For now the docs tell users to keep alpha_range inside it. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Local full suite: PASS (6 min, Julia 1.13.0, one cell of the matrix) |
TL;DR
A geometry YAML entry
[id, masure_regression, {t, eta, kappa, delta, lambda, phi}]now resolves to aPOLAR_VECTORStable throughresolve_aero_geometry(...; ml_models_dir), the wayneuralfoilentries do. Before,resolve_airfoilthrew "masure_regression airfoils are not yet supported". The Python VSM already supports this airfoil type. Here the model runs in Julia insideAirfoilAero, and gives the same coefficients as scikit-learn to the last bit.How the model gets into Julia
The Masure regression is three scikit-learn pickles on Zenodo, about 2.3 GB each, one per Re (1e6, 5e6, 2e7). The Python VSM loads them with
pickleand callspredict, whose output columns are CD, CL, CM. The pickles are not in the Python repo, and Julia cannot read them.The training code (
jellepoland/WES_aero_sim_for_kite_design,utils_from_kasper.py) buildsPipeline(StandardScaler, MultiOutputRegressor(ExtraTreesRegressor(n_estimators=130, max_depth=30))). The Python VSM's compatibility patch walks the same shape.scripts/export_masure_models.pyunpickles that layout and writes the scaler's mean and scale, plus each output's trees, toET_re<Re>.npz. Each output's trees are stored as five flat arrays. Any other layout raises. This is a one-off step and needs numpy and scikit-learn.src/airfoil_aero/masure_regression.jlloads the file and caches it per model path, likeload_neuralfoil_model. It walks each tree to its leaf and averages the leaves in tree order, asExtraTreesRegressor.predictdoes. An Re with no model, or a missing file, errors with the Zenodo DOI and the export command. An entry missing one of the six parameters errors naming the airfoil id and the absent keys, instead of a bareKeyError.The one trap: sklearn compares in Float32
sklearn casts the scaled input to
Float32before testingx <= threshold, and the threshold staysFloat64.scale_inputdoes the same. Without the cast, a point near a split can take the other branch.The test fixture includes rows moved onto each output's first root split to catch this. With the comparison done in
Float64, 5 of the 29 fixture rows fail. With the cast, all 29 match sklearn'spredictexactly, and the test checks them with==.Also changed
Extra-Trees return a constant outside the alpha range they were trained on, so a masure airfoil left on the default
alpha_rangeof -180:180 gets flat tails with no warning. The exported model does not record its training range, so the docs say to keepalpha_rangewithin it. A warning would need the range added to the export format, which is left for later.write_polar(filepath, alpha, cl, cd, cm)is now the single writer. TheSectionSolutionandNeuralFoilResultmethods call it instead of each repeating thewrite_node_rowscall.Typos in
data/TUDELFT_V3_KITE: "lamba" → "lambda", and the type spelled "measure_regression" → "masure_regression".On review of the plan, @1-Bart-1 asked whether the npz conversion is hard to get right, and whether a full-Julia version could be compared against it directly. That comparison is the parity test against sklearn above. Retraining the trees in Julia from Masure's CFD data could only agree statistically, so it is not in this PR.
Searched for
masure,ExtraTrees,regression,predict,write_polarandwrite_node_rowsbefore writing. The only prior masure code was the error stub, and the only otherwrite_node_rowspolar writer was the two methods now unified.Verification
resolve_airfoil("masure_regression", …)ends inerror("masure_regression airfoils are not yet supported by AirfoilAero.").test/airfoil_aero/test_masure_regression.jl: 37/37. Red with the Float32 cast removed: 5 of the 29 parity rows fail. The missing-parameter test was red before its check:Message: "KeyError: key \"lambda\" not found", 36 passed, 1 failed.resolve_aero_geometry→WingloadingPOLAR_VECTORSwith the predicted cl.test/obj_adapter/test_obj_adapter.jl(269/269, before the review round) andtest/airfoil_aero/test_airfoil_aero.jl(covers thewrite_polarrefactor).scripts/export_masure_models.py --fixturein apython:3.12-slimcontainer with scikit-learn 1.9.1: 5 trees of depth ≤ 6 per output, 47 KB. Re-running it after the review round's exporter change gives byte-identical files.agent ci-local, one cell, Julia 1.13.0): PASS on 7132853 in 11 min, 7330 passed, 1 broken (pre-existing), masure regression 37/37. An earlier run reported FAIL withKeyError: key "lambda" not found: it started at 14:08:49Z, after the missing-parameter test was written and before its fix was committed at 14:17:39Z, so it ran the test's red-before state. GitHub CI: PASS: Julia 1.12 on ubuntu, macOS and windows, Julia 1.13 on ubuntu, the docs, the setup test and codecov/patch. Branch is up to date with main.Scope
+373 / −34 across 17 files, of which:
src/airfoil_aero/masure_regression.jl: +117, the evaluator.scripts/export_masure_models.py: +125, export and fixture generation..npzfiles (50 KB).resolve_airfoil/resolve_aero_geometrywiring, thewrite_polarunification, docs, changelog and the YAML typo fixes.Opened by
1-Bort-1, an AI agent working for @1-Bart-1.Closes #387 · task
VortexStepMethod.jl-387