aesara
aesara copied to clipboard
Fix C++11 narrowing error on Mac OS
Resolves #127
Previously, tests including the following would fail with the following errors:
FAILED tests/graph/test_compute_test_value.py::TestComputeTestValue::test_empty_elemwise - aesara.link.c.exceptions.CompileError: Compilation failed (return status=1):
FAILED tests/graph/test_compute_test_value.py::TestComputeTestValue::test_overided_function - aesara.link.c.exceptions.CompileError: Compilation failed (return status=1):
FAILED tests/graph/test_compute_test_value.py::TestComputeTestValue::test_scan_err1 - aesara.link.c.exceptions.CompileError: Compilation failed (return status=1):
FAILED tests/graph/test_compute_test_value.py::TestComputeTestValue::test_scan_err2 - aesara.link.c.exceptions.CompileError: Compilation failed (return status=1):
TODO: Add tests to verify modification of config.gcc__cxxflags
- [ ] No
gcc_cxxflags- Darwin -> Add
-Wno-c++11-narrowing - Other
sys.platform-> No-Op
- Darwin -> Add
- [ ] Has
gcc__cxxflagsbut no-Wno-c++11-narrowing- Darwin -> Add
-Wno-c++11-narrowingand other flags preserved - Other platform -> other flags preserved
- Darwin -> Add
- [ ] Has
gcc_cxxflagsincluding-Wno-c++11-narrowing- All platforms ->
gcc__cxxflagsremain the same
- All platforms ->
Codecov Report
Merging #972 (40d12ff) into main (ead2c02) will decrease coverage by
0.02%. The diff coverage is100.00%.
:exclamation: Current head 40d12ff differs from pull request most recent head 2bef458. Consider uploading reports for the commit 2bef458 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## main #972 +/- ##
==========================================
- Coverage 74.75% 74.73% -0.02%
==========================================
Files 194 194
Lines 49896 49880 -16
Branches 10553 10550 -3
==========================================
- Hits 37298 37278 -20
- Misses 10270 10273 +3
- Partials 2328 2329 +1
| Impacted Files | Coverage Δ | |
|---|---|---|
| aesara/link/c/cmodule.py | 52.32% <100.00%> (+0.27%) |
:arrow_up: |
| aesara/tensor/__init__.py | 95.55% <0.00%> (-0.75%) |
:arrow_down: |
| aesara/tensor/math.py | 90.42% <0.00%> (-0.27%) |
:arrow_down: |
| aesara/scalar/basic.py | 79.04% <0.00%> (-0.13%) |
:arrow_down: |
| aesara/scalar/__init__.py | 100.00% <0.00%> (ø) |
We do this by default in pymc: https://github.com/pymc-devs/pymc/blob/main/pymc/init.py#L36 it should be pretty safe to just always do that.
I was going to make this a bit fancier by checking the existing flags gcc_cxxflags on a platform-dependent basis, and only adding the compatibility flags as necessary. I don't think it's an issue to have duplicate flags (if the user has already included them), but does add a bit of noise.
Still need to add tests to aesara whether we use the pymc code @twiecki linked to or my implementation. @brandonwillard Any preference?
In general, we should try to apply OS/arch-specific settings only to said OS/arch's, whenever possible.
I should probably check that we're on macOS and using the C backend. This probably doesn't apply with the numba backend.
Also, I need to add a test case for this.
@brandonwillard Is there an existing location for config-related test cases?
@brandonwillard Is there an existing location for config-related test cases?
tests.test_config is for the general configuration classes, but there really isn't a place for testing specific config settings. Since this config setting is specific to C compilation, tests.link.c.test_basic is probably a suitable place.