My original title and reason for opening this issue was thatftorch_int leaked into the public interface via the layout argument.
However, digging into it I thing there are other issues stemming from ftorch_int.
Effects have so-far gone unnoticed, likely because ftorch_int (and c_int used for other subroutine inputs) is int32, matching the typical integer on most systems.
It is currently used only as the layout input to ttfa However, for proper compatibility with the rest of the API we should make this c_int, consistent with other input arguments.
I think the documented usage as being changable for "large size tensors" is also false, as layout holds only dimension numbers, not dimension sizes. Indeed, tensor shapes and strides are already hard-coded as c_int64_t.
Also, changing ftorch_int to int64 as instructed in the docs breaks the build!
One option would be to just remove the ftorch_int from the codebase and change to int32/c_int as it doesn't fulfill the stated purpose in the docs.
However, I notice when looking at this that a lot of the public API uses integer(c_int) (or integer(c_int64_t) for shape).
For a public Fortran API it feels like these should really be integer or precisions from iso_fortran_env.
So there is perhaps an argument for a much larger change to make all subroutine inputs integer or int32 and then rely on an internal casting to c_int for the torch_tensor_from_blob call (and zeros, ones, empty).
This would then prevent any compatibility issues where users with a different default integer kind need to import iso_c_binding and cast to c_int just to use FTorch. Or indeed see the examples where shapes are declared as c_int64_t.
We could also consider keeping ftorch_int and using it where we currently have c_int64_t to match what is described in the docs.
However, are we ever going to see a tensor with more than 2,147,483,647 elements in a single dimension...??? That'd be 6mm global resolution! Edit: Torch defaults to int64 everywhere, so perhaps we should match this.
These changes could go in now as part of V2 - realistically no-one will notice, but sets us up better for future?
To summarise I think my preferred approach is:
- Remove
ftorch_int from codebase, replacing by explicit int32
- Make all interfaces use Fortran types (
int32, int64 etc.), not C-interoperability types (c_int, c_int64_t etc.)
- This will require adding an explicit cast to C-types at
blob, zeros, ones, and empty C-boundaries but makes the API Fortran native.
- examples will be updated to use
iso_fortran_env: int64 instead of iso_c_binding: c_int64_t
- get_shape and get_stride will have to return
int_64
Add back ftorch_int replacing instances of int64 (currently c_int64_t) for shape, satisfying its original stated purpose and defaulting to int32. Edit, maybe don't bother, see note above.
Side note to also fix: reduction_type in loss functions should be c_int as passed a C enum.
My original title and reason for opening this issue was that
ftorch_intleaked into the public interface via thelayoutargument.However, digging into it I thing there are other issues stemming from
ftorch_int.Effects have so-far gone unnoticed, likely because
ftorch_int(andc_intused for other subroutine inputs) isint32, matching the typicalintegeron most systems.It is currently used only as the layout input to
ttfaHowever, for proper compatibility with the rest of the API we should make thisc_int, consistent with other input arguments.I think the documented usage as being changable for "large size tensors" is also false, as
layoutholds only dimension numbers, not dimension sizes. Indeed, tensor shapes and strides are already hard-coded asc_int64_t.Also, changing
ftorch_inttoint64as instructed in the docs breaks the build!One option would be to just remove the
ftorch_intfrom the codebase and change toint32/c_intas it doesn't fulfill the stated purpose in the docs.However, I notice when looking at this that a lot of the public API uses
integer(c_int)(orinteger(c_int64_t)for shape).For a public Fortran API it feels like these should really be
integeror precisions fromiso_fortran_env.So there is perhaps an argument for a much larger change to make all subroutine inputs
integerorint32and then rely on an internal casting toc_intfor thetorch_tensor_from_blobcall (andzeros,ones,empty).This would then prevent any compatibility issues where users with a different default integer kind need to import
iso_c_bindingand cast toc_intjust to use FTorch. Or indeed see the examples where shapes are declared asc_int64_t.We could also consider keepingEdit: Torch defaults toftorch_intand using it where we currently havec_int64_tto match what is described in the docs.However, are we ever going to see a tensor with more than 2,147,483,647 elements in a single dimension...??? That'd be 6mm global resolution!
int64everywhere, so perhaps we should match this.These changes could go in now as part of V2 - realistically no-one will notice, but sets us up better for future?
To summarise I think my preferred approach is:
ftorch_intfrom codebase, replacing by explicitint32int32,int64etc.), not C-interoperability types (c_int,c_int64_tetc.)blob,zeros,ones, andemptyC-boundaries but makes the API Fortran native.iso_fortran_env: int64instead ofiso_c_binding: c_int64_tint_64Add backEdit, maybe don't bother, see note above.ftorch_intreplacing instances ofint64(currentlyc_int64_t) for shape, satisfying its original stated purpose and defaulting toint32.Side note to also fix:
reduction_typein loss functions should bec_intas passed a C enum.