Skip to content

Modernize legendre_convert Boost usage - #56

Merged
egull merged 6 commits into
modernize/step2from
modernize/step2.3b
Sep 24, 2026
Merged

egull merged 6 commits into
modernize/step2from
modernize/step2.3b

Conversation

@egull

@egull egull commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add deterministic golden regression coverage for legendre_convert
  • replace Boost.Random with the C++ standard <random> implementation
  • replace boost::math::factorial with a small local product
  • replace boost::math::legendre_p with the standard three-term recurrence
  • intentionally retain Boost spherical Bessel and Boost.Program_options

Verification

  • Debug build with warnings-as-errors passes
  • legendre_convert regression passes in Debug and Release
  • transform and convergence outputs are bit-identical; back-continuation differs only by 5.1e-18
  • complete regression-fast label passes
  • git diff --check passes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved moderate issues remain in the implementation and regression tolerance configuration.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Modernizes legendre_convert by replacing selected Boost functionality with standard C++ implementations and adding deterministic regression coverage.

Changes:

  • Replaces Boost.Random, factorial, and Legendre polynomial usage.
  • Adds regression inputs, references, tolerances, and CTest integration.
  • Documents migration progress and reference provenance.
File Review summary
test/​regression/​tolerances.json Moderate, 1 vote: Tolerance key does not match the generated legendre_convert_transform:<dataset> keys.
test/​regression/​REFERENCE_CHANGES.md Reviewed; no final comment.
test/​regression/​README.md Reviewed; no final comment.
test/​regression/​PROVENANCE.md Reviewed; no final comment.
test/​regression/​manifest.py Reviewed; no final comment.
test/​regression/​MANIFEST.md Reviewed; no final comment.
test/​regression/​make_inputs.py Reviewed; no final comment.
test/​regression/​inputs/​legendre_convert_transform/​input.dat Reviewed; no final comment.
test/​regression/​generate.py Nit, 1 vote: The missing-program diagnostic suggests an invalid underscore-form option.
test/​regression/​compare.py Reviewed; no final comment.
test/​regression/​CMakeLists.txt Reviewed; no final comment.
test/​regression/​cases.py Reviewed; no final comment.
legendre_convert/​legendre_convert.cpp Moderate, 1 vote: Required standard headers are not included directly. Moderate, 3 votes: Zero error bars create std::normal_distribution with an invalid zero standard deviation.
ALPS3_MIGRATION_PLAN.md Reviewed; no final comment.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread legendre_convert/legendre_convert.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The migrated random-distribution path is not exercised by the regression coverage.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Low severity Add shell continuation to multiline command example

test/​regression/​generate.py:7

This newly added command example is not shell-continuable: without a trailing \\ here, the next [--provenance ...] line is executed as a separate command. Add the continuation character so the documented invocation can be copied verbatim.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

Only documentation nits remain; no blocking issues were identified.

Review effort: Lite
Findings: None

@egull
egull merged commit 454f7ee into modernize/step2 Sep 24, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants