Cytnx-dev / Cytnx

Project Cytnx, A Cross-section of Python & C++,Tensor network library
Apache License 2.0
35 stars 13 forks source link

User include files should not depend on library configuration (especially USE_GPU) #502

Open ianmccul opened 5 days ago

ianmccul commented 5 days ago

A small number of files that are installed as part of the public API include some conditional code hidden behind

#if defined(UNI_GPU)

This should not appear in header files in the user API in include/.

  1. Network.hpp This conditionally adds variables with type cutensornetNetworkDescriptor_t and cutensornetContractionOptimizerInfo_t to the Network_base class. This means it will produce an ABI incompatibility if UNI_GPU and UNI_CUQUANTUM in user code is different to how it was configured when the library was installed. Better option would be to put this behind an opaque type, say, NetworkImplementation (forward declared as an incomplete type in the header) and replace the cuda types with std::unique_ptr<NetworkImplementation>. (Note there are some minor technicalities with having a std::unique_ptr to an incomplete type, but it does work, and was designed to be allowed.) 2.utils/cucomplex_arithmetic.hpp This is included from utils/utils.hpp. It declares a bunch of overloads for equality comparison and operator* between cuComplexand builtin types. I have no idea why it exists -- user code using cytnx normally wouldn't ever need to deal with cuFloatComplex and cuDoubleComplex directly.
  2. utils/complex_arithmetic.hpp
  3. Not sure why this header exists either - it declares a bunch of overloads for arithmetic operations between complex<double> / complex<float> and builtin types (double, float, int etc). Most of them already exist in namespace std anyway, but a few of them do not (which I guess is the purpose of the header...). Since they are declared in namespace cytnx, user code typically would not ever find them (unless using namespace cytnx, and then it is asking for trouble with two clashing overloads). Some of them are a bit strange, eg what does cytnx_complex64 operator/(const cytnx_bool &rn, const cytnx_complex64 &ln) do?! [edit: actually I think all of them exist in namespace std, via implicit conversion from builtin types to complex] But it also includes a copy-and-paste of the cuComplex operator== declarations that also appear in utils/cucomplex_arithmetic.hpp (but not the operator* overloads...)
  4. cytnx_error.hpp This looks like a lot of it was copy-pasted from https://github.com/NVIDIA/cuda-samples/blob/master/Common/helper_cuda.h . None of the CUDA-related functions are referred to anywhere else in the include files, so it looks like these could simply be removed from the public API. (also _cudaGetErrorEnum() could be replaced by the CUDA function cuGetErrorString(). But I suggest simply importing helper_cuda.h from the CUDA samples into the src/ directory and using it as-is.
yingjerkao commented 5 days ago

We should clean up the code base...

yingjerkao commented 16 hours ago

I also propose to put only public API header files in cytnx/include and keep the private headers next to the cpp files. In this way we have a clear separation of user level API and internal functions.