search

Cambridge-ICCS / FTorch

A library for directly calling PyTorch ML models from Fortran.
https://cambridge-iccs.github.io/FTorch/
MIT License
79 stars 16 forks source link

Use assertions rather than print statements in tests #166

Closed jwallwork23 closed 1 month ago

jwallwork23 commented 2 months ago

Closes #142.

As part of this PR, we introduce a module for test utils, e.g., printing test results and making assertions.

jwallwork23 commented 2 months ago

One discussion to have about these changes is whether we're happy that the examples are becoming more verbose. The versions without assertions will still be used in the docs but (e.g.) SimpleNet now has some extra stuff in there that might confuse things for beginners.

jwallwork23 commented 2 months ago

Huh! The CI has spontaneously hit a shape error again https://github.com/Cambridge-ICCS/FTorch/actions/runs/10880645825/job/30187925536

As suggested by @TomMelt, we should use valgrind to see if there's a memory leak.

jatkinson1000 commented 2 months ago

I'm getting a similar issue over in #173 I haven't touched the test, but ab getting[ERROR]: Array allocated with wrong shape from the torch_tensor_to_array() subroutine.

Digging into this the error seems to occur when processing the shape of an unallocated Fortran array. It seems that all(shape(data_out) == 0) returns F, but not because shape(data_out) is all zeros - indeed, calling shape(data_out) following this leads to a segfault due to an invalid memory reference for me.

I think I have found a fix which is to test (.not. associated(data_out)) instead. I have implemented this in https://github.com/Cambridge-ICCS/FTorch/pull/173/commits/e2b581df20156ce4cce4e71965625e35c2bc3fee as part of #173 which this may need rebasing on if merged. Hopefully it will help here, though I am not 100% certain as the errors seem like they could be subtly different.

jatkinson1000 commented 2 months ago

Over in #174 I have modified the workflow to dump the error log from the failed test and output the arguments causing it to fail in the fortran.

See attached screenshot - despite sizes being 2 out_data seems to be allocated to have shape 95:

Screenshot 2024-10-01 at 10 50 45

Whereas a 'random success' correctly sets both to 2:

image
jwallwork23 commented 1 month ago

Merged in @TomMelt's fixes from #175 and extended the test utils to consider other shapes than just 1D arrays. Now ready for review!

jatkinson1000 commented 1 month ago

Also looks like you may need want to do a squash and merge (or interactive rebase your end, but squash acceptable here I think, and probably causes fewer tears).