Upgrade VMD_cpp into a reusable library and CLI - #2
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the repository from a legacy single-binary implementation into an installable C++17 library (VMD_cpp::vmd) plus a CLI (vmd_cli), while preserving the original VMD.h / VMD(...) compatibility wrapper and significantly expanding correctness, documentation, and CI coverage.
Changes:
- Introduces a new public API (
include/vmd/vmd.hpp,vmd::decompose) with structured options/results and improved numerical correctness/stability. - Adds a CLI with robust CSV/TSV parsing, CSV/JSON outputs, and a self-contained HTML report, plus unit + end-to-end tests.
- Modernizes build/packaging (CMake 3.20+, install/export,
find_packagesupport) and replaces the legacy MSBuild workflow with a CMake-based CI matrix.
Reviewed changes
Copilot reviewed 38 out of 41 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| VMD.h | Replaces legacy header contents with a compatibility wrapper interface over the new vmd::decompose API. |
| VMD.cpp | Removes the old standalone example main() implementation. |
| VMD_Utils.cpp | Reimplements legacy VMD(...) by translating arguments into vmd::Options and delegating to vmd::decompose; updates circshift. |
| VMD_cpp.vcxproj.filters | Removes obsolete Visual Studio filters file. |
| VMD_cpp.vcxproj | Removes obsolete Visual Studio project file. |
| VMD_cpp.sln | Removes obsolete Visual Studio solution file. |
| tests/vmd_tests.cpp | Adds unit tests covering spectra contract, edge cases, determinism, warm starts, validation, and the legacy wrapper. |
| tests/package_consumer/main.cpp | Adds a downstream consumer smoke test for the installed/exported CMake package. |
| tests/package_consumer/CMakeLists.txt | Adds CMake project that consumes VMD_cpp via find_package. |
| tests/cli_e2e.cmake | Adds CTest-driven end-to-end validation for the CLI outputs, error handling, and report structure. |
| src/vmd.cpp | Adds the core VMD implementation behind vmd::decompose with improved stability, options, and diagnostics. |
| src/fft_backend.hpp | Introduces an FFT backend abstraction (Eigen default; optional FFTW/MKL). |
| src/fft_backend.cpp | Implements the FFT backend abstraction for Eigen/FFTW/MKL. |
| README.md | Rewrites project documentation for library/CLI usage, options, outputs, install, and validation. |
| include/vmd/vmd.hpp | Adds the new public API types (Options, Result, Diagnostics, etc.) and the decompose entry point. |
| examples/vmd_example.cpp | Adds a minimal example program using the new library API. |
| Eigen/Eigen/src/Core/util/NonMPL2.h | Restores missing vendored Eigen header(s) for Windows CI parity. |
| Eigen/Eigen/src/Core/BooleanRedux.h | Restores missing vendored Eigen header(s) for Windows CI parity. |
| Eigen/Eigen/src/Core/arch/SYCL/SyclMemoryModel.h | Restores missing vendored Eigen header(s) for Windows CI parity. |
| Eigen/Eigen/src/Core/arch/Default/TypeCasting.h | Restores missing vendored Eigen header(s) for Windows CI parity. |
| Eigen/Eigen/src/Core/arch/CUDA/Complex.h | Restores missing vendored Eigen header(s) for Windows CI parity. |
| Eigen/.gitignore | Anchors ignore rules to avoid case-insensitive collisions on Windows (e.g., Core). |
| docs/validation-and-performance.md | Adds documented validation coverage and benchmark guidance/results. |
| docs/summary.schema.json | Adds a JSON schema for the CLI summary output. |
| docs/cli-and-formats.md | Adds CLI usage and output format specification. |
| docs/api.md | Adds API reference documentation for the new library and legacy wrapper. |
| docs/algorithm.md | Adds algorithm notes detailing frequency layout, mirroring, stopping criteria, and backends. |
| CMakeLists.txt | Replaces old build with a modern CMake project: library target, CLI, tests, benchmarks, install/export, optional docs/backends. |
| cmake/VMD_cppConfig.cmake.in | Adds installed-package config with backend dependency discovery (Eigen/FFTW/MKL). |
| benchmarks/vmd_benchmark.cpp | Adds a small benchmark executable reporting timings and workspace estimate. |
| app/vmd_cli.cpp | Adds the CLI entry point with argument parsing, execution, serialization, and exit codes. |
| app/report.hpp | Declares HTML report generation. |
| app/report.cpp | Implements self-contained offline HTML/SVG report generation with downsampled envelopes. |
| app/cli_io.hpp | Declares CLI I/O, parsing helpers, CSV writers, and summary writer. |
| app/cli_io.cpp | Implements CSV/TSV parsing, alpha parsing, output generation, and JSON summary writing. |
| .gitmodules | Removes obsolete submodule configuration. |
| .gitignore | Adds ignores for local build/output directories. |
| .github/workflows/msbuild.yml | Removes obsolete MSBuild-based workflow. |
| .github/workflows/ci.yml | Adds CMake-based CI matrix (MSVC/GCC/Clang) + install-consumer + sanitizers. |
Suppressed comments (3)
src/fft_backend.cpp:80
- In the FFTW backend, ensure_real() updates real_size before creating the r2c plan. If fftw_plan_dft_r2c_1d fails, the instance retains real_size plus allocated buffers but a null plan, so later calls may incorrectly early-return. Use temporaries and only set real_size/real_input/real_output/real_forward_plan after successful plan creation (or reset on failure).
src/fft_backend.cpp:105 - In the MKL backend, ensure_complex() assigns complex_size before descriptor creation/commit. If any DFTI call fails and throws, complex_size remains equal to requested while complex_descriptor may be null or partially initialized, causing later calls with the same size to early-return and use an invalid descriptor. Clear complex_descriptor/complex_size up front and only set complex_size after successful commit; free any partially created descriptor on failure.
src/fft_backend.cpp:121 - In the MKL backend, ensure_real() assigns real_size before descriptor creation/commit. If creation fails, real_size can remain at requested while real_descriptor is null/invalid, so later calls may early-return and use a bad descriptor. Reset real_descriptor/real_size before attempting creation and only commit real_size after successful commit; free any partially created descriptor on failure.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+56
to
+70
| void ensure_complex(std::size_t requested) { | ||
| if (requested == complex_size) return; | ||
| reset_complex(); | ||
| complex_size = requested; | ||
| complex_input = static_cast<fftw_complex*>(fftw_malloc(sizeof(fftw_complex) * complex_size)); | ||
| complex_output = static_cast<fftw_complex*>(fftw_malloc(sizeof(fftw_complex) * complex_size)); | ||
| if (complex_input == nullptr || complex_output == nullptr) throw std::bad_alloc(); | ||
| forward_plan = fftw_plan_dft_1d( | ||
| static_cast<int>(complex_size), complex_input, complex_output, FFTW_FORWARD, FFTW_ESTIMATE); | ||
| inverse_plan = fftw_plan_dft_1d( | ||
| static_cast<int>(complex_size), complex_input, complex_output, FFTW_BACKWARD, FFTW_ESTIMATE); | ||
| if (forward_plan == nullptr || inverse_plan == nullptr) { | ||
| throw std::runtime_error("failed to create FFTW complex plan"); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This upgrade turns VMD_cpp into a reusable C++17 library and command-line application while retaining the legacy
VMD(...)compatibility wrapper.What changed
vmd::decomposeAPI with independent alpha values, deterministic random seeds, warm starts, adaptive tau, and omega history. The existingVMD.h/VMD(...)interface remains available.vmd_cliwith CSV/TSV input, CSV/JSON output, and a self-contained HTML report.find_packagesupport, CI, examples, benchmarks, and algorithm/API/CLI/format/validation/performance documentation.Local validation performed
MSVC Debug / Eigen FFT
F:\VMD_cpp\out\build\x64-Debugsuccessfully through the Visual Studio x64 developer environment.vmd.unit,vmd.cli.e2e).vmd_example.exe: exit 0; 8 x 1200 modes, 314 iterations, converged, reconstruction error1.63551e-05.vmd_tests.exe: exit 0;All VMD tests passed.3.16564894809836e-05.modes.csv,spectra.csv,omega.csv,summary.json, andreport.html; JSON parsed successfully and numeric CSV fields were finite.Fresh MSVC Release / Eigen FFT
find_package(VMD_cpp 2 CONFIG REQUIRED)consumer configured, built, and ran successfully.MinGW GCC Release / Eigen FFT
Vendored Eigen / CI parity check
.gitignorematched theCoredirectory case-insensitively on Windows.-DVMD_USE_VENDORED_EIGEN=ON, built successfully, and passed CTest 2/2.Optional backend status
jsonschemavalidation was not run because that package is not installed; JSON parsing and the project's CTest schema-fragment checks passed.Compatibility and migration notes
<vmd/vmd.hpp>andvmd::decompose;VMD.hand the legacy wrapper remain supported.epsargument is still accepted for source compatibility but remains ignored.u_hatoutput can differ from results produced by the previous buggy spectrum implementation.