Long long fix

[TomMelt]

closes #183

Problem

This issue also affected windows and is also fixed by this PR.

Tldr; FTorch expects a long int* whilst the libtorch binary seems to be returning a long long int*.

Example output below.

[ 25%] Building CXX object CMakeFiles/ftorch.dir/ctorch.cpp.o
/Users/user/FTorch/src/ctorch.cpp: In function 'const long int* torch_tensor_get_sizes(torch_tensor_t)':
/Users/user/FTorch/src/ctorch.cpp:240:25: error: invalid conversion from 'const long long int*' to 'const long int*' [-fpermissive]
  240 |   return t->sizes().data();
      |          ~~~~~~~~~~~~~~~^~
      |                         |
      |                         const long long int*

Solution

The solution is to check which system we are running on (OS and architecture) and create a preprocess macro -DUNIX that we can use to switch between long long int and long int versions of the libtorch library.

FTorch/src/CMakeLists.txt

Lines 57 to 62 in 5fb0887

	if(UNIX)
	message(STATUS "CMAKE_SYSTEM_PROCESSOR = ${CMAKE_SYSTEM_PROCESSOR}")
	if(CMAKE_SYSTEM_PROCESSOR STREQUAL "x86_64")
	target_compile_definitions(${LIB_NAME} PRIVATE UNIX)
	endif()
	endif()

Then in the source code we check for that macro e.g.,

FTorch/src/ctorch.h

Lines 128 to 132 in 5fb0887

	#ifdef UNIX
	EXPORT_C const long int* torch_tensor_get_sizes(const torch_tensor_t tensor);
	#else
	EXPORT_C const long long int* torch_tensor_get_sizes(const torch_tensor_t tensor);
	#endif

TODO

• works on windows
• works on linux
• works on mac (M1/M2)
• create windows CI (this will be a separate PR) Windows CI build #168

[TomMelt]

@jwallwork23 @jatkinson1000 , to make reviewing easier this PR should be merged before #137

[TomMelt]

Suggested change

message(STATUS "CMAKE_SYSTEM_PROCESSOR = ${CMAKE_SYSTEM_PROCESSOR}")

Just remembered I left this print statement in for debugging. I should take this out

[jatkinson1000]

Did you remove this?

[TomMelt]

not yet. I will look at fixing the mac build before I take it out completely

[jwallwork23]

This looks sensible to me, @TomMelt. I haven't tested it on my laptop running Windows but I can do if needed.

[TomMelt]

This looks sensible to me, @TomMelt. I haven't tested it on my laptop running Windows but I can do if needed.

Thanks @jwallwork23. Wouldn't hurt if you get time but I did test on windows vm so it should be fine 👌

I do plan on setting up a windows CI soon so that should also catch any issues.

[jatkinson1000]

It seems that this incompatibility still exists on Mac 12 and 13 - see #192

So the CMake/#ifdef might need some refinement.
IIRC 12 and 13 are X86 images rather than arm, but I'd need to check that.

[jwallwork23]

I'm surprised the linter likes these not being on the same line.

[TomMelt]

honestly... it makes me sad. I had them on the same line, but the linter forces it to split :(

[jatkinson1000]

Can maybe use a config file to extend the column limit?
https://clang.llvm.org/docs/ClangFormatStyleOptions.html

[jatkinson1000]

I'm a little confused, when I run clang-format locally it moves them to be placed on one line...

[jwallwork23]

I wonder if this is to do with the default settings being different for different clang-format versions? We observed this in nextSIM-DG and so pinned the version used by the CI.

[jatkinson1000]

I have just opened #199 to here which should resolve this by fixing Cpp Column Limit to 88.

[jwallwork23]

Approved once, will approve again. Thanks @TomMelt!

[jatkinson1000]

As discussed in-person, I think this can go in now and the issues with mac can be resolved in a separate set of issues.

Thanks @TomMelt!

[jatkinson1000]

Have rebased #199 on this if you want to merge.