Skip to content

Removal of ftorch_int? #619

Description

@jatkinson1000

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions