sahrk / DGGRID

A command-line application that generates and manipulates icosahedral discrete global grids.
GNU Affero General Public License v3.0
78 stars 26 forks source link

Use more explicit variable types #36

Closed r-barnes closed 3 years ago

r-barnes commented 3 years ago

The code has a lot of things like unsigned long long int. This results in variable length data types depending on the compiler and hardware used. Where the range of these values is important it would be nice to have the code use consistent and self-explanatory data types such as:

sahrk commented 3 years ago

My intent with the long long data types was so that DGGRID would automatically be able to generate the highest resolution grids directly supported by the language on whatever platform you're compiling on. So if unsigned long long int is 32 bits, you can generate grids with up to 2^32 cells. But if you're on a machine where it's 64 bits, then you can generate grids up to 2^64 cells, etc.

chardan commented 3 years ago

Maybe not completely germane here, but definitely related if you're doing any compile-time generations, C++14 added a feature called "variable templates" that might be useful to bear in mind. For example, you can compute PI to the maximum precision available for a given data type. C++17 further lets you more easily do what amounts to enable-if to select an implementation, so you could write something like (pseudocode):

if constexpr (is_same_v<int32_t, my_int_prec>) { calc_for_32(params...); }

I hope that's more helpful than just noisy, sorry to appear out of nowhere. :-)

See-also: https://en.cppreference.com/w/cpp/language/variable_template

r-barnes commented 3 years ago

You could look into intmax_t or uintmax_t (full list here: https://en.cppreference.com/w/cpp/header/cstdint).

But dropping 32-bit support in favour of code simplicity seems like a win to me.

Microsoft is no longer supporting 32-bit: https://www.thespectrum.com/story/news/2020/05/25/pc-periodicals-end-near-32-bit-windows-10/5254933002/

ARM is going to drop all 32-bit production: https://www.arm.com/blogs/blueprint/64-bit

32-bit is still out there for embedded devices, &c, but that's kind of niche.

DGGRID doesn't seem to have a strong test suite associated with it, so I suspect the 32-bit maximum integer size hasn't been thoroughly tested in some time. If that's true, then doubling down on the 64-bit assumption might be safer than assuming a 32-bit mode works.

One counter-point is that GPUs can still get significantly higher throughput on 32-bit integers versus 64-bit, but I don't think it'll be easy to integrate DGGRID with CUDA to get GPU offloading. (For S2 it's pretty trivial, but that was part of its mathematical design.)

sahrk commented 3 years ago

Thanks @r-barnes, I understand the advantage of having fixed sizes for porting to other languages. But our intent was always to have DGGRID generate the highest resolution grids at the highest efficient precision available, given the computing environment available to a researcher. We wanted something a researcher in a poor country could run on their 32 bit Windows NT machine (if only they can get it to build), but that would also adapt to a 128-bit super computer. For now I think I want to stay with that approach.

sahrk commented 3 years ago

Thanks @chardan, that sounds like a useful mechanism for the future.