WIP: Standalone C-level tests for 1D convolution (GSoC 2026)#19452
WIP: Standalone C-level tests for 1D convolution (GSoC 2026)#19452toastmt wants to merge 12 commits intoastropy:mainfrom
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
How do you run it in CI? How do you ask user to run it? |
Hi @pllim, For the actual GSoC project, the goal will be to automate this. |
|
I don't think this idea is sustainable. astropy is a Python package first. It is enough to test it via the Python interface. |
This is in response to the methodology outlined in the official GSoC 2026 project idea, "Hardening astropy’s core stability," which proposes creating a fundamental test suite for the low-level C layer, in order to make it more independent from the existing astropy tests. (https://openastronomy.org/gsoc/gsoc2026/#/projects?project=hardening_astropy's_core_stability) I am happy to adjust the technical approach of my proposal to align with whatever consensus the maintainer team reaches regarding the testing infrastructure. |
|
Sorry for being so late. I think this approach is absolutely valid and desirable.
to restate the context: this GSoC proposal is a subset of a larger proposal to make astropy itself pure Python, but this means moving the compiled components to a different package, which is only reasonable if they have sufficient test coverage that they can be decoupled from the Python interface. So, before pushing this, @toastmt, how would you propose to integrate this to our automated test runners ? |
|
Hi @neutrinoceros. If we don't want to write a Cython wrapper for testing (in order to keep the C tests decoupled from Python) we could add a compile step directly in the CI workflow to compile the C testing suite into standalone executables and then have python evaluate the tests via subprocess. That way the C testing suite would be prepared for a future decoupling of the C-core or a migration to Meson. |
|
That seems reasonable to me. Feel free to give it a try. |
This will annoy me and probably other devs because it polutes the bin space for no reason. I stress again #19452 (comment) |
|
Okay let's reframe this as an experiment then; |
I doubt it. I maintain a C pipeline elsewhere. |
|
I've been giving this a try. I've implemented a pytest script that evaluates the C tests via subprocess. I created a new job in the YAML workflow file to compile the C test binary and then run the pytest file. I also inserted a mock NumPy header creation step in the CI workflow to decouple the C compilation from the Python environment. The remaining hurdle is fully isolating pytest from astropy's pyproject.toml and conftest.py, which pull in the full package. Open to suggestions on the cleanest approach here. |
|
|
||
| test_c_extensions: | ||
| # Standalone tests for the C math extensions of Astropy | ||
| # Create a mock ndarrayobject header to decouple C logic from Python/NumPy |
There was a problem hiding this comment.
I would expect test binaries to not need numpy at all. The fact that you need a mock header to make it work makes me doubt the value of the approach; it's not decoupled if you need a mock.
There was a problem hiding this comment.
I agree that a mock header isn't true decoupling. The issue is that the mock header is providing the type definitions that convolve.c needs. The alternative would be refactoring convolve.c to use standard C types like double instead of npy_float64, but that approach touches core C files across the repository. Is there a different approach you'd recommend for achieving true decoupling without modifying the core?
There was a problem hiding this comment.
I think it makes sense to slightly edit the C modules in this way (replacing a type alias with its primitive definition): the benefit of decloupling outweights the cost of refactoring, in my opinion. That is, I think it does, but maybe I'm not seeing the complete picture yet. Could you explain the benefit(s) of using npy_float64 instead of double ?
There was a problem hiding this comment.
npy_float64 is effectively just a typedef for double. However, npy_float64 guarantees 64-bit precision across all systems and platforms, while double doesn't. We can use a _Static_assert in the header to guarantee that with a double too. I could try to refactor the convolve.h header slightly to decouple the C file from NumPy.
There was a problem hiding this comment.
I'm sorry about the needlessly long delay in my reply.
npy_float64 guarantees 64-bit precision across all systems and platforms, while double doesn't.
this feels like news to me. I might have learned that a while back, but I did not recall it, nor was I able to quickly find a source to explain this nuance. Would you by chance have a resource you could point me to ?
I could try to refactor the convolve.h header slightly to decouple the C file from NumPy.
sounds good to me, assuming you feel confident it won't take you long.
There was a problem hiding this comment.
this feels like news to me. I might have learned that a while back, but I did not recall it, nor was I able to quickly find a source to explain this nuance. Would you by chance have a resource you could point me to ?
In "C Programming: A Modern Approach" by K.N. King, page 132 it says that most modern computers follow the IEEE Standard 754 (IEC 60559) which defines double precision at 64 bits, which implies not all systems do, and it's not a strict C requirement. Following on that, I found on the ARM website that "Both x86 and Arm architectures fully implement the IEEE 754 standard and produce identical results for all well-defined floating-point operations." So it seems to me that probably all systems using Astropy would define a double to be 64 bits.
Looking at the NumPy npy_common.h header, I found that NumPy actually checks the sizes of float, double and long double in the system, and then assigns npy_float64 to the type that matches 64 bits to ensure the correct size. So maybe a static assert would be a redundancy? But might be good to add as a safety check.
sounds good to me, assuming you feel confident it won't take you long.
Ok, I'll give it a try.
There was a problem hiding this comment.
Thanks a lot for this explanation, really appreciate it !
So maybe a static assert would be a redundancy? But might be good to add as a safety check.
yes. Better safe than sorry, especially here !
There was a problem hiding this comment.
I've been looking into this. I'm considering adding a flag (#ifdef C_STANDALONE #else) in the convolve header, to create two build paths, one that would use npy_float64 during a standard Astropy build, and one that would use a double + _Static_assert for standalone compilation. To support this, I would need to add a macro definition for the flag in the build scripts. Touching the build configuration itself feels a bit too much to me right now, but could be a clean way to have a standalone C build, in the sense that after defining the flag I could apply it to any files we want to include in the standalone build. What do you think?
EDIT: Actually, thinking this more, by using an #ifdef C_STANDALONE block, I can trigger the standalone path simply by passing -DC_STANDALONE in the compilation command and then I wouldn't have to touch the build file. I'll try it out and see if it works.
…o isolate from Python package dpendencies
|
This seems to work now. I used a C_STANDALONE compilation flag in convolve.h to bypass NumPy and Python library includes during the standalone gcc build. The binary test with subprocess is now executed directly (python test_c_binaries.py) rather than through pytest, which prevents pytest from pulling in the full package as a dependency. It seems to me that this approach (compiled C binaries tested via a Python subprocess wrapper) could be scaled to use for all of the C-core files across the codebase. |
neutrinoceros
left a comment
There was a problem hiding this comment.
Awesome, I really like the direction you took.
I don't think I'll be able to merge this promptly, because as we saw earlier the approach itself might be controversial and I need to build consensus for it, but I personally appreciate it a lot.
I have a simple question and a general level suggestion left:
- I couldn't find much about
C_STANDALONE. Is this a standard, a convention ? - the python module should be normally run with
pytestinstead of just throughpython
| if result.stdout: | ||
| print(result.stdout) | ||
|
|
||
| if result.stderr: | ||
| print(result.stderr, file=sys.stderr) |
There was a problem hiding this comment.
we shouldn't ever use print in a test (unless testing print-related behavior, that is).
I suggest only using printf for errors, so return 0 never should output anything and we can simply check
| if result.stdout: | |
| print(result.stdout) | |
| if result.stderr: | |
| print(result.stderr, file=sys.stderr) | |
| assert not result.stdout | |
| assert not result.stderr |
There was a problem hiding this comment.
Ok, I will change this and the c test file to only print errors
| if result.stderr: | ||
| print(result.stderr, file=sys.stderr) | ||
|
|
||
| error_msg = ( |
There was a problem hiding this comment.
is there a point to binding the error message to a variable that I'm missing ?
There was a problem hiding this comment.
No, you are correct that this has no real point. I will delete this and make it more streamlined.
| assert result.returncode == 0, error_msg | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
I get that this could have been useful in early stages but we shouldn't keep this around now.
There was a problem hiding this comment.
I use the main block here to execute this file directly, in order to keep the testing suite as decoupled from Python as possible. I can remove the block and transition the workflow to use pytest, but doing so will require re-introducing Python dependencies and we would have to install the full Python dependency stack into this specific CI job. Wouldn't this negate the goal of having a zero-dependency standalone test environment?
Yes, I completely understand that, this is more of an attempt to define some way forward with this project.
No, C_STANDALONE is not a standard; it's just a name I chose for the preprocessor macro to be descriptive of its function in the workflow, could use anything in its place. It acts as a conditional switch to exclude the Python/NumPy headers during the standalone build.
Regarding pytest, the test was set to run directly with Python (python test_c_binaries.py) to prevent pytest from pulling in the Python configuration files. Running it through pytest attempts to initialize the whole astropy package tree, which requires each of those packages to be installed in the CI environment, defeating the goal of a zero-dependency C-core test. If there is a pytest configuration that isolates the execution without triggering package imports, I can adjust the workflow accordingly. I just couldn't find a way to do this in a simple way. |
|
hum, maybe these test shouldn't be integrated to within the source tree |
Doing this could cleanly solve the import isolation problem! I'll give it a try. |
|
Ok, this now works with pytest. Moving everything to the ctests folder solved the numpy import problem. I also had to use --noconftest to prevent pytest from loading astropy's conftest.py, and created a pytest.ini file inside ctests/ to stop pytest from picking up astropy's pyproject.toml. It runs in CI with pytest --noconftest ctests. It would also work with uv run --no-project -w pytest pytest --noconftest ctests , but I kept the CI job simpler since the runner environment is already isolated. |
Description
This Draft PR serves as a proof-of-concept accompanying my GSoC 2026 proposal: Hardening Astropy’s Core Stability.
I have implemented a standalone C testing file (test_convolve_1d.c) that directly interfaces with the convolve1d_c function in astropy/convolution/src/convolve.c.
What this code does:
Bypassing the Cython wrapper (_convolve.pyx), this test file directly validates the memory handling and mathematical edge cases of the C-core. It currently tests:
Standard array convolution accuracy.
NaN interpolation handling.
The bot=0 fallback behavior when encountering arrays comprised entirely of NaNs.
Because this is a standalone executable, it can be compiled and run directly from the src directory without building the entire Python package.
I am looking forward to any feedback on this approach!