-
Notifications
You must be signed in to change notification settings - Fork 0
Replace dense matrix spline implementation with efficient sparse matrix approach #40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
krystophny
wants to merge
56
commits into
main
Choose a base branch
from
faster-spline
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
56 commits
Select commit
Hold shift + click to select a range
b2893ec
Fast default splines
krystophny de8f46b
Fix natural boundary conditions
krystophny d6513c1
Fix index of spline coefficient
krystophny 13d1755
Add error handling for LAPACK call
krystophny 7115ce9
Remove trailing whitespaces
krystophny 276d766
Add spline module design analysis documentation
krystophny 8ba7a92
Replace splinecof3_a with direct sparse matrix implementation
krystophny 3b534aa
Replace dense matrix implementation with direct sparse implementation
krystophny e164751
Fix testing scope by removing enable_testing() from COMMON
krystophny d1ebc3d
Update make test to run only spline comparison test
krystophny ba1bd46
WIP: Add runtime performance test infrastructure
krystophny 52d656d
Revert complex module restructuring, keep sparse implementation
krystophny 9ea1ab9
Restore inter_interfaces.f90 to main branch state
krystophny a28d6bb
Add fast path detection to main spline_cof.f90 and update documentation
krystophny f04088f
Fix tests to properly validate correctness against original implement…
krystophny 0c9594e
Improve test validation to only compare when dispatch conditions are met
krystophny 3f2ddf3
Fix fast path bug by disabling incompatible optimization
krystophny c437997
Clean up PR and fix critical buffer overflow vulnerability
krystophny 6f0e619
Fix GitHub Actions to avoid duplicate runs and respect draft status
krystophny 5ef3f82
Update Splines.md documentation to reflect final implementation status
krystophny 82c3fb4
Clean up Splines.md to focus on design rather than development process
krystophny 2f721d7
Address QODO and GeorgGrassler PR review concerns
krystophny 4063de1
Improve sparse matrix memory allocation with exact analytical calcula…
krystophny ea9f46b
Fix critical bugs identified by QODO code review
krystophny 30feacc
Add unit tests to CI workflow
krystophny f51d463
Fix matrix construction bugs in sparse spline implementation
krystophny d9863a5
Address QODO review concerns and add fast path optimization
krystophny 2bb9515
Fix fast spline implementation with correct boundary conditions
krystophny de30349
Fix mathematical errors in clamped boundary second derivatives
krystophny 3bd5f4d
make clean
krystophny 7e1b97c
case sensitive
krystophny b02c52e
Fix clamped boundary conditions in fast spline implementation
krystophny 133f1e6
Add analytical tests proving original clamped boundary bug
krystophny 691064e
Debug and enhance spline tests, prove original implementation bug
krystophny b811387
Update comparison tests to handle original implementation bug
krystophny 1535172
Document and maintain boundary condition limitation in spline impleme…
krystophny b814861
Fix sparse spline implementation to produce identical results to dense
krystophny d51f494
Add comprehensive code coverage tracking with lcov and Codecov integr…
krystophny 4fec172
Restructure CI: separate unit tests with coverage from integration tests
krystophny 22621e5
Improve CI workflows: fix MPI caching issues and optimize triggers
krystophny 02b035a
Add liblapack-dev to cached packages in both workflows
krystophny 724a695
Align codecov.yml with fortfront patch coverage requirements
krystophny f8d4c4d
Cache safe MPI dependencies to speed up CI
krystophny 38d3251
Fix CI build failures: ensure pkg-config files for BLAS and MPI
krystophny ae6de8a
Add use_fast_splines configuration option (default: false)
krystophny 4e6c0f2
Consolidate spline documentation into DOC/DESIGN/Splines.md
krystophny 7f0fac8
Update documentation with use_fast_splines configuration option
krystophny 7db8507
Fix module naming and configuration structure
krystophny 4dc49a1
Simplify spline configuration by removing obsolete disable_fast_path
krystophny 93aa297
Simplify spline configuration by removing obsolete disable_fast_path
krystophny 5c4fb7e
Add comprehensive coverage test for sparse spline implementation
krystophny d909be8
Add targeted test for uncovered error paths to improve code coverage
krystophny a081dbb
Add coverage and test output files to gitignore
krystophny 2bbad17
Add patch coverage tests targeting 0% coverage files
krystophny 9a6946b
Remove dead code that was never used and hurting patch coverage
krystophny 2b307ae
cleanup
krystophny File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| name: Unit Tests with Coverage | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
| - master | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, ready_for_review] | ||
| branches: | ||
| - main | ||
| - master | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| unit-tests-coverage: | ||
| runs-on: ubuntu-24.04 | ||
| if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.draft == false) | ||
|
|
||
| env: | ||
| CC: gcc | ||
| CXX: g++ | ||
| FC: gfortran | ||
| MPI_HOME: /usr | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Cache apt packages | ||
| uses: awalsh128/cache-apt-pkgs-action@v1 | ||
|
Check warning on line 34 in .github/workflows/unit-tests-coverage.yml
|
||
| with: | ||
| packages: git cmake make ninja-build gcc g++ gfortran jq lcov libopenblas-dev liblapack-dev libsuitesparse-dev libfftw3-dev libgsl-dev libhdf5-dev libnetcdf-dev libnetcdff-dev python3-dev libevent-dev libnuma-dev libhwloc-dev libnl-3-dev libnl-route-3-dev libltdl-dev openmpi-bin openmpi-common libopenmpi-dev | ||
| version: 1.0 | ||
| execute_install_scripts: true | ||
|
|
||
| - name: Ensure BLAS and MPI are properly configured | ||
| run: | | ||
| # Fallback installation to ensure pkg-config files are present | ||
| sudo apt-get update | ||
| sudo apt-get install -y --reinstall libopenblas-dev pkg-config | ||
| sudo apt-get install -y --reinstall openmpi-bin openmpi-common libopenmpi-dev | ||
|
|
||
| # Verify BLAS can be found | ||
| pkg-config --exists openblas && echo "OpenBLAS pkg-config found" || echo "Warning: OpenBLAS pkg-config not found" | ||
|
|
||
| # Verify MPI can be found | ||
| pkg-config --exists ompi && echo "OpenMPI pkg-config found" || echo "Warning: OpenMPI pkg-config not found" | ||
| which mpifort && echo "mpifort found at: $(which mpifort)" || echo "Warning: mpifort not found" | ||
|
|
||
| # Set environment variables to help CMake | ||
| echo "BLA_VENDOR=OpenBLAS" >> $GITHUB_ENV | ||
| echo "MPI_HOME=/usr" >> $GITHUB_ENV | ||
|
|
||
| - name: Set up Python 3 | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install numpy lcov-cobertura | ||
|
|
||
|
|
||
|
|
||
| - name: Build NEO-2 with coverage flags | ||
| run: | | ||
| cmake --preset default -DCMAKE_BUILD_TYPE=Coverage | ||
| cmake --build --preset default | ||
|
|
||
| - name: Run unit tests with coverage | ||
| run: | | ||
| make test | ||
|
|
||
| - name: Generate coverage report | ||
| run: | | ||
| # Capture coverage data | ||
| cd build && lcov --capture --directory . --output-file coverage.info \ | ||
| --rc branch_coverage=1 \ | ||
| --rc geninfo_unexecuted_blocks=1 \ | ||
| --ignore-errors inconsistent,mismatch,empty,unused | ||
|
|
||
| # Filter coverage data to include only source code | ||
| lcov --remove coverage.info \ | ||
| '*/build/*' \ | ||
| '*/TEST/*' \ | ||
| '*/libneo/*' \ | ||
| '*/thirdparty/*' \ | ||
| '*/DOC/*' \ | ||
| '*/MULTI-SPEC-TOOLS/*' \ | ||
| '*/tools/*' \ | ||
| '/usr/*' \ | ||
| '/tmp/*' \ | ||
| --output-file coverage_filtered.info \ | ||
| --rc branch_coverage=1 \ | ||
| --ignore-errors unused,empty | ||
|
|
||
| # Generate XML for Codecov | ||
| lcov_cobertura coverage_filtered.info -o coverage.xml | ||
|
|
||
| # Show coverage summary | ||
| echo "=== Coverage Summary ===" >> $GITHUB_STEP_SUMMARY | ||
| lcov --summary coverage_filtered.info >> $GITHUB_STEP_SUMMARY || echo "No coverage data found" >> $GITHUB_STEP_SUMMARY | ||
|
|
||
| - name: Upload coverage to Codecov | ||
| uses: codecov/codecov-action@v4 | ||
|
Check warning on line 105 in .github/workflows/unit-tests-coverage.yml
|
||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| file: ./build/coverage.xml | ||
| flags: unittests | ||
| name: unit-tests-coverage | ||
| fail_ci_if_error: false | ||
| verbose: true | ||
|
|
||
| - name: Upload coverage artifacts | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: coverage-reports | ||
| path: | | ||
| build/coverage.info | ||
| build/coverage_filtered.info | ||
| build/coverage.xml | ||
| retention-days: 7 | ||
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have
masterbranch now?