Protect `torch_tensor_from_array` from abuse

[Mikolaj-A-Kowalski]

I think there are two potentially dangerous things that can be done with the from_array tensor constructors. Perhaps we should try to think about some protections or at least we should probably discuss these failure modes more explicitly in the documentation?

Passing non-contiguous arrays is allowed

Currently there is nothing that protects against doing the following (e.g. in the SimpleNet example)

   ...
   ! Set up Fortran data structures
   real(wp), dimension(10), target :: in_data
   real(wp), dimension(10), target :: out_data
   real(wp), dimension(10), target :: expected
   ...

   ! Create Torch input/output tensors from the above arrays
   call torch_tensor_from_array(in_tensors(1), in_data(1:10:2), torch_kCPU)
   call torch_tensor_from_array(out_tensors(1), out_data(1:10:2), torch_kCPU)
   ...

which (unless I am forgetting something) is valid Fortran and something a user may do at some point. The problem is that the result is not what is expected since the torch_tensor_from_array implicitly assumes that the input array is contiguous. As the result in this example the in_data(1:5) will be used instead of the odd indices.

Perhaps we should add is_contiguous check inside torch_tensor_from_array and throw an error if it isn't?

Passing a temporary array

Since the data_in dummy argument is declared intent(in) the following is allowed:

   call torch_tensor_from_array(in_tensors(1), in_data*2, torch_kCPU)

as in we can provide result of an expression as an argument, which I think will make a tensor point to a temporary. This problem is technically mentioned in the docs (temporary array has no target attribute which the docs say is required), but I feel like this is easy to miss (e.g. some of the usage like that sneaked into the unit tests) and we should think of some automatic protection against it.

I think there are two legal ways of doing that:

1. (preferred) Make the data_in an intent(in) pointer. I believe in that case a target actual argument should still bind. Also maybe adding contiguous attribute will help to move the check from the 1st problem from runtime to compile-time (need to check) [EDIT: Seems to work on the first glance. The only problem is that the error message is not very informative. Just: `"There is no specific subroutine..." :-/
2. We can make the data_in intent(inout) and just be careful not to modify it by accident. It should prevent accidental usage with temporary arrays (or at least reduce a chance of that happening)

Last but not least I am not sure how it will all work for backwards compatibility :-/

[Mikolaj-A-Kowalski]

1. (preferred) Make the data_in an intent(in) pointer. I believe in that case a target actual argument should still bind. Also maybe adding contiguous attribute will help to move the check from the 1st problem from runtime to compile-time (need to check) [EDIT: Seems to work on the first glance. The only problem is that the error message is not very informative. Just: `"There is no specific subroutine..." :-/

I think it should be a reasonably safe solution. I made this simple test program and run it through the usual suspects of the Fortran compilers (gfortran, ifort, ipx, flang(!), LFortran) and only LFortran fails to reject it.

The compiler messages are OK... but only if the procedure is not part of a generic interface 😢

[jatkinson1000]

Thank you @Mikolaj-A-Kowalski

Passing non-contiguous arrays

Perhaps we should add an is_contiguous check inside torch_tensor_from_array and throw an error if it isn't?

Yes please, and I think this is fine for backwards compatibility as we are "fixing" to enforce described behaviour. I don't see how someone would be exploiting this "feature".
It should be explicitly added to the docs/docstrings that we the routine takes a contiguous array though (or, well, a pointer to an array in memory from which stuff gets sorted out).

Also adding the contiguous attribute is a sensible suggestion I think. [EDIT: Unless this goes towards solving the second point as well.]

I suggest doing this in its own PR if you are willing.

Passing a temporary array

You mention there is an unhelpful error message - perhaps we can catch and provide a more intuitive error message for when a subroutine match is not found listing potential things to be checked (contiguous, not operation, appropriate data type...)

(e.g. some of the usage like that sneaked into the unit tests)

Which ones?

[Mikolaj-A-Kowalski]

Yes please, and I think this is fine for backwards compatibility as we are "fixing" to enforce described behaviour. I don't see how someone would be exploiting this "feature".
It should be explicitly added to the docs/docstrings that we the routine takes a contiguous array though (or, well, a pointer to an array in memory from which stuff gets sorted out).

Also adding the contiguous attribute is a sensible suggestion I think. [EDIT: Unless this goes towards solving the second point as well.]

I suggest doing this in its own PR if you are willing.

Overall changing interface to pointer, contiguous :: ... seems to fix both problems. Basically on all established compilers code will not compile.

For the backwards compatibility I only fear that there might be an existing code somewhere that forgot the target attribute on actual argument, in most cases it will run fine I think since the Fortran array and Tensor operations are quite separate so problems related to aliasing should be unlikely (I guess). With the change of the procedure interface it will now not compile.
I suppose it should be OK within the scope of a minor release or patch. Sadly SemVer is not really helpful with regards to guidance... What exactly is "backwards-compatible bug fix" ?! 🤔

I will open a PR about it soon then ;-)

You mention there is an unhelpful error message - perhaps we can catch and provide a more intuitive error message for when a subroutine match is not found listing potential things to be checked (contiguous, not operation, appropriate data type...)

I fear that we cannot really do much about the error message since it comes from the compiler. Basically it complains that it does not find the match for a procedure call inside the generic interface (which is true but sadly compilers seem not to provide any suggestions for 'closest matching' option that would help to debug what is going on). I think that the best we can do is to highlight this case in the documentation (doccomment + TensorAPI doc page).

Which ones?

We will get a full list when I refactor them in the PR. But here is an example from unit tests and here one from examples ;-)