-
Notifications
You must be signed in to change notification settings - Fork 76
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
Reimplement managed raster class in C++ #1597
Changes from 83 commits
5e58f60
7c51c08
a55d57c
068d886
8576088
7f2ec80
11ef40f
c7486f2
048e769
cccf0e6
8d104a4
6ca0f78
61d1f65
d3aebac
eb15fa1
93ec4b9
4506414
1ff72b6
6acaea6
9316d20
0d4805a
53b15d0
06c8648
1749eb9
e4c234f
bbd7c5a
7bc4b58
efc5527
474a89a
a63532f
05aa968
81af8a3
a0e5810
d5bd998
09a6f8e
c669180
61883f5
d236af1
7e12302
c664e43
baac039
8cf1e6b
5932fdb
8fe87ec
d7d3b09
4e5e717
5f6a647
8defb6f
6245b6a
364d7ff
386beb2
7b994e2
f62a696
8109a60
da23f3e
31565e8
8c31a12
b88567c
36c26b2
fc8dd38
e6a370e
2cf8f5d
ba37a0a
b957881
989a7b6
b8e710f
b532001
9421e2c
9981569
6d556e2
c7742e8
7b9db1f
9071362
ba9cebe
13cc0dc
7a767f5
5b8c04e
947900c
f737225
3a623eb
420145f
dde78df
11cc0ef
25c954a
d50a2c9
3adae2d
24d1028
b3d2a55
45ec188
9b95796
3c9b795
07059b0
041cd1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,9 +20,18 @@ | |
# access to a pre-Mavericks mac, so hopefully this won't break on someone's | ||
# older system. Tested and it works on Mac OSX Catalina. | ||
compiler_and_linker_args = [] | ||
if platform.system() == 'Darwin': | ||
compiler_and_linker_args = ['-stdlib=libc++'] | ||
|
||
include_dirs = [numpy.get_include(), 'src/natcap/invest/managed_raster'] | ||
if platform.system() == 'Windows': | ||
compiler_args = ['/std:c++20'] | ||
library_dirs = [f'{os.environ["CONDA_PREFIX"]}/Library/lib'] | ||
include_dirs.append(f'{os.environ["CONDA_PREFIX"]}/Library/include') | ||
else: | ||
library_dirs = [] | ||
compiler_args = [] | ||
compiler_and_linker_args = ['-stdlib=libc++', '-std=c++20'] | ||
library_dirs = [subprocess.run( | ||
['gdal-config', '--libs'], capture_output=True, text=True | ||
).stdout.split()[0][2:]] # get the first argument which is the library path | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are now building directly against There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm... the libraries are a dependency of the build, but the |
||
|
||
class build_py(_build_py): | ||
"""Command to compile translation message catalogs before building.""" | ||
|
@@ -50,13 +59,14 @@ def run(self): | |
Extension( | ||
name=f'natcap.invest.{package}.{module}', | ||
sources=[f'src/natcap/invest/{package}/{module}.pyx'], | ||
include_dirs=[numpy.get_include()] + ['src/natcap/invest/managed_raster'], | ||
extra_compile_args=compiler_args + compiler_and_linker_args, | ||
include_dirs=include_dirs, | ||
extra_compile_args=compiler_args + package_compiler_args + compiler_and_linker_args, | ||
extra_link_args=compiler_and_linker_args, | ||
language='c++', | ||
libraries=['gdal'], | ||
library_dirs=library_dirs, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe GDAL also has a self-configuration utility that might be able to help us find the location of its include directory: https://gdal.org/en/latest/programs/gdal-config.html I don't know that this will help us out on Windows, but it might at least be nicer on linux and mac. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh neat! Thanks for pointing that out. I'm not sure what the solution would be for Windows, to avoid relying on conda - possibly we'd need to make the GDAL lib location configurable as an env variable |
||
define_macros=[("NPY_NO_DEPRECATED_API", "NPY_1_7_API_VERSION")] | ||
) for package, module, compiler_args in [ | ||
('managed_raster', 'managed_raster', []), | ||
) for package, module, package_compiler_args in [ | ||
('delineateit', 'delineateit_core', []), | ||
('recreation', 'out_of_core_quadtree', []), | ||
# clang-14 defaults to -ffp-contract=on, which causes the | ||
|
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.
Is C++20 truly the minimum language version required? I don't think this should be a huge deal since compilers seem to pretty rapidly adopt the new standards, so I think this is mostly a question of curiosity.
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.
Good question,
I think I used some C++20 features during development, but now only need C++17. (It still compiles with 11, but I get warnings that the inline variables are a C++17 feature.) Updated in d50a2c9C++20 is needed on the Windows build only, for some reason, otherwise there are a lot of syntax errors, e.g. https://github.com/natcap/invest/actions/runs/12245867222/job/34160661279There 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.
Sounds good! Thanks for looking into it.
If I had to guess, maybe the lack of an
--ffp-contract=off
flag (needed for addressing a compilation bug in viewshed) is causing the rest of the errors? If (big if) that's the only thing standing in the way, then the viewshed tests would fail, which isn't worth it if all we need is a recent enough version of the C++ standard.