Improve pulscan integration and GPU robustness#286
Conversation
- Introduced new source file `aa_device_pulscan.cu` containing CUDA kernels for Pulscan processing. - Updated `aa_pipeline_runner.hpp` to include Pulscan candidate retrieval method, conditionally compiled based on `AA_WITH_PULSCAN`. - Modified `meson.build` to add compilation flag for Pulscan support. - Included `aa_device_pulscan.cu` in the build source list. - Implemented various kernels for data processing, normalization, and candidate searching specific to Pulscan.
- Implemented test for calculating logp in `test_pulscan_calculate_logp.cpp` and corresponding CMake configuration. - Added sorting test for pulscan candidates in `test_pulscan_candidate_sort.cpp` with CMake setup. - Created decimation harmonics test in `test_pulscan_decimate_harmonics.cpp` and its CMake configuration. - Developed helper functions test in `test_pulscan_helpers.cpp` with appropriate CMake integration. - Introduced magnitude squared test in `test_pulscan_magnitude_squared.cpp` and its CMake setup. - Added median normalization test in `test_pulscan_median_normalisation.cpp` with CMake configuration. - Integrated pipeline testing in `test_pulscan_pipeline_integration.cpp` and corresponding CMake setup. - Created separate components test in `test_pulscan_separate_components.cpp` with CMake configuration.
There was a problem hiding this comment.
Pull request overview
This PR completes the in-tree Pulscan integration (compile-time gated), adds GPU robustness fixes (notably corner-turn large-grid handling), and extends the repository’s Pulscan-focused test and tooling support.
Changes:
- Add Pulscan CUDA kernels + an
aa_pulscan_runnerand plumb Pulscan candidates through the generic pipeline, CLI config, and disk outputs. - Improve GPU robustness by chunking the corner-turn kernel launch along
grid.ywith an offset parameter to avoid exceeding CUDA grid limits. - Add Pulscan regression/unit tests, sample configs, docs, and a SIGPROC filterbank splitting helper script.
Reviewed changes
Copilot reviewed 52 out of 53 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_pulscan_separate_components.cpp | New GPU unit test for FFT component separation kernel. |
| tests/test_pulscan_separate_components.cmake | CMake harness for the separate-components test. |
| tests/test_pulscan_requires_periodicity.cpp | New integration test ensuring Pulscan does not run without periodicity enabled. |
| tests/test_pulscan_requires_periodicity.cmake | CMake harness for the “requires periodicity” test. |
| tests/test_pulscan_pipeline_integration.cpp | New pipeline-level Pulscan integration test (candidate ordering, end-to-end run). |
| tests/test_pulscan_pipeline_integration.cmake | CMake harness for the pipeline integration test. |
| tests/test_pulscan_median_normalisation.cpp | New GPU unit test for median/MAD normalisation kernel. |
| tests/test_pulscan_median_normalisation.cmake | CMake harness for the normalisation test. |
| tests/test_pulscan_magnitude_squared.cpp | New GPU unit test for magnitude-squared kernel. |
| tests/test_pulscan_magnitude_squared.cmake | CMake harness for the magnitude-squared test. |
| tests/test_pulscan_helpers.cpp | New unit tests for Pulscan helper math (logp→sigma). |
| tests/test_pulscan_helpers.cmake | CMake harness for helper-math tests. |
| tests/test_pulscan_decimate_harmonics.cpp | New GPU unit test for harmonic decimation kernel. |
| tests/test_pulscan_decimate_harmonics.cmake | CMake harness for harmonic decimation test. |
| tests/test_pulscan_compatibility_helpers.cpp | New tests for compatibility helpers (FFT bin truncation behavior). |
| tests/test_pulscan_compatibility_helpers.cmake | CMake harness for compatibility helper tests. |
| tests/test_pulscan_candidate_sort.cpp | New unit test for candidate sorting/trimming behavior. |
| tests/test_pulscan_candidate_sort.cmake | CMake harness for candidate sort test. |
| tests/test_pulscan_calculate_logp.cpp | New GPU unit test for logp computation kernel. |
| tests/test_pulscan_calculate_logp.cmake | CMake harness for calculate-logp test. |
| tests/test_pulscan_boxcar_filter.cpp | New GPU unit test for boxcar filter candidate extraction (incl. short-input regression). |
| tests/test_pulscan_boxcar_filter.cmake | CMake harness for boxcar filter test. |
| tests/CMakeLists.txt | Conditionally includes Pulscan tests behind AA_ENABLE_PULSCAN. |
| src/aa_main.cpp | Adds Pulscan disk output plumbing and directory creation for Pulscan outputs. |
| src/aa_device_pulscan.cu | New CUDA implementation of Pulscan kernels and kernel launch wrappers. |
| src/aa_device_corner_turn_kernel.cu | Adds block_y_offset to corner-turn kernel to support chunked grid.y launches. |
| src/aa_corner_turn.cpp | Chunks corner-turn launches to stay within CUDA grid.y limit (65535). |
| scripts/split_filterbank.py | New helper to split SIGPROC .fil files into smaller chunks. |
| meson_srclist.txt | Adds Pulscan CUDA source to Meson source list. |
| meson.build | Adds Pulscan compile-time define toggled via Meson option (requires option definition). |
| input_files/ska_test_file.txt | Simplifies sample config content (now references a specific local path). |
| input_files/pulscan_test.cfg | New Pulscan-oriented sample configuration. |
| input_files/pulscan_sample.cfg | New Pulscan sample configuration. |
| input_files/pulscan_repro_no_periodicity.cfg | New repro config for running without periodicity. |
| input_files/pulscan_generated.cfg | New sample config for generated input. |
| input_files/pulscan_dm30125_part000_delivery.cfg | New delivery config incl. Pulscan output directory setting. |
| input_files/pulscan_dm30125_part000.cfg | New sample config for a fixed DM run. |
| input_files/pulscan_dm30125.cfg | New sample config for a fixed DM run (full file). |
| include/aa_pulscan_runner.hpp | New Pulscan runner (FFT batching, candidate extraction, host-side accumulation/sorting). |
| include/aa_pipeline_runner.hpp | Adds Pulscan candidate access/output methods to pipeline runner interface (guarded). |
| include/aa_pipeline_api.hpp | Exposes Pulscan candidate access/output methods via pipeline API (guarded). |
| include/aa_pipeline.hpp | Adds failsafe pipeline option and description string. |
| include/aa_permitted_pipelines_generic.hpp | Integrates Pulscan runner into generic pipeline and adds failsafe propagation + cleanup updates. |
| include/aa_params.hpp | Defines AA_ENABLE_PULSCAN default (0) if not provided by build system. |
| include/aa_device_pulscan.hpp | New Pulscan public kernel wrapper declarations + candidate structs. |
| include/aa_device_info.hpp | Makes device clock-rate reporting robust across CUDA runtime versions. |
| include/aa_device_corner_turn_kernel.hpp | Updates corner-turn kernel wrapper signatures to include block_y_offset. |
| include/aa_config.hpp | Parses pulscan-output-dir and wires failsafe into pipeline options/config struct. |
| README.md | Adds Pulscan build/usage documentation and output descriptions. |
| Makefile | Adds PULSCAN=1 toggle mapping to AA_ENABLE_PULSCAN. |
| MANUAL.md | Documents Pulscan as an experimental feature and how to enable it. |
| CMakeLists.txt | Adds CMake option for Pulscan, bumps C++ standard flags, adjusts CUDA linking. |
| .gitignore | Ignores Pulscan output artifacts (*.gpucand, dumps, CSV). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if get_option('with_pulscan') | ||
| add_project_arguments(['-DAA_ENABLE_PULSCAN=1'], language : ['c', 'cpp', 'cuda']) | ||
| else | ||
| add_project_arguments(['-DAA_ENABLE_PULSCAN=0'], language : ['c', 'cpp', 'cuda']) | ||
| endif |
| sigma_cutoff 6 | ||
| periodicity | ||
| dedispersion | ||
| file /home/jay/workspace/aa/pulscan/sample_data/1298us_binary_from_dat.fil |
| copy_ddtr_data_to_host | ||
| threshold | ||
| failsafe | ||
| file /mnt/data/jay/ska_split/1_part000.fil |
| copy_ddtr_data_to_host | ||
| threshold | ||
| failsafe | ||
| file /mnt/data/jay/aa_pulscan_repro/pulscan_repro_part000.fil |
| copy_ddtr_data_to_host | ||
| threshold | ||
| failsafe | ||
| file /home/jay/workspace/aa/pulscan/generated/binary_pulsar.fil |
| while(runner.run(status_code)) { | ||
| if(status_code == aa_pipeline_runner::status::error) { | ||
| std::cout << "Pipeline reported error status" << std::endl; | ||
| break; | ||
| } |
| copy_ddtr_data_to_host | ||
| threshold | ||
| failsafe | ||
| file /home/jay/workspace/aa/pulscan/sample_data/1298us_binary_multi32_8bit.fil |
| def read_string(fp) -> str: | ||
| (length,) = struct.unpack('<i', fp.read(4)) | ||
| if length <= 0: | ||
| return '' | ||
| return fp.read(length).decode('ascii') |
| list(APPEND CUDA_NVCC_FLAGS --use_fast_math) | ||
| list(APPEND CUDA_NVCC_FLAGS -g;) | ||
| list(APPEND CUDA_NVCC_FLAGS -Xptxas -O3 -std=c++11;) | ||
| list(APPEND CUDA_NVCC_FLAGS -Xptxas -O3 -std=c++14;) | ||
| list(APPEND CUDA_NVCC_FLAGS -lineinfo;) | ||
| list(APPEND CUDA_NVCC_FLAGS -Xcompiler;-O3;) | ||
| list(APPEND CUDA_NVCC_FLAGS -Xcompiler;-fopenmp;) | ||
| list(APPEND CUDA_NVCC_FLAGS -Xcompiler;-lm;) | ||
| list(APPEND CUDA_NVCC_FLAGS -Xcompiler;-Wall;) | ||
| list(APPEND CMAKE_CXX_FLAGS "-std=c++11 -O3 -lm -Wall -Wpedantic -Wextra -fopenmp") | ||
| list(APPEND CMAKE_CXX_FLAGS "-std=c++14 -O3 -lm -Wall -Wpedantic -Wextra -fopenmp") |
| timer.Start(); | ||
| m_pulscan_runner.clear(); |
There was a problem hiding this comment.
I would prefer to leave ska_test_file.txt in previous form, partly because the options are not really well documented and "normal user" would have to go to the source code of astroaccelerate to get full list of switches.
There was a problem hiding this comment.
that makes sense; reverted
| add_definitions(-DASTRO_ACCELERATE_LOGGING_FACILITY_ENABLE=0) | ||
| endif() | ||
|
|
||
| option(AA_ENABLE_PULSCAN "Enable Pulscan integration" OFF) |
There was a problem hiding this comment.
Is there a reason to have pulscan integration off by default? Looking at the configuration files it seems that it cannot be enabled from there. Is that correct? Which means if I want pulscan on I need to recompile. Can a switch be added to enable pulscan from configuration file?
|
Looks good, sorry that it took like two months or more to look at it. |
Summary
Finish the current Pulscan integration work and ship the GPU robustness fixes into
master.Highlights
AA_ENABLE_PULSCANValidation
astro-accelerate.filand confirmed the abnormal large candidates disappearedbuild-tests/tests/test_pulscan_boxcar_filter