Add realistic example

[ElliottKasoar]

Fixes #32

Replaces the example ones tensor with a more realistic example, based on a Pytorch Resnet example.

To do:

• Update resnet_infer_fortran.f90 to read in the saved text file for the image
• Update resnet_infer_fortran.f90 to highest probabilities with labels (possibly via saving the loaded labels in python, in a similar manner to the image array?)

[TomMelt]

Thanks for starting this. It looks great so far 👍

[jatkinson1000]

Thanks for working on this @ElliottKasoar
A couple of comments:

• Whilst it is arguably 'cleaner' in terms of storage to fetch the image from url, there is no guarantee it will always be available. Since it is only a single image I would consider just adding it to the repository to make the example self-contained. This will also arguably make the python a bit cleaner.
• Saving the data to file is a good idea, but could we do it more nicely than .flatten().to_file()? And consider adding *.dat to .gitignore. Also a comment in the docs about writing/reading between row/column major systems for clarity could be really helpful for users' understanding.

[ElliottKasoar]

* Saving the data to file is a good idea, but could we do it more nicely than `.flatten().to_file()`? And consider adding `*.dat` to `.gitignore`. Also a comment in the docs about writing/reading between row/column major systems for clarity could be really  helpful for users' understanding.

@jatkinson1000 What sort of thing would you consider to be nicer? My feeling was that flattening it would make it the least ambiguous, or do you mean the way it is flattened/saved? (I'll add a commend about writing/reading anyway)

[ElliottKasoar]

* Whilst it is arguably 'cleaner' in terms of storage to fetch the image from url, there is no guarantee it will always be available. Since it is only a single image I would consider just adding it to the repository to make the example self-contained. This will also arguably make the python a bit cleaner.

What about something in between (which I'm halfway along, having added the .jpg to the PR): try to download the data, but, if necessary, catch the HTTPError for both the image and the categories, as we can direct users to the pre-downloaded files.

I quite like having some form of the fetch present, as, for example, I wanted to test a different dog, which only requires changing one line of code at the moment, and I think it makes an example better if the barrier to trying out different data is as low as possible.

[jatkinson1000]

* Whilst it is arguably 'cleaner' in terms of storage to fetch the image from url, there is no guarantee it will always be available. Since it is only a single image I would consider just adding it to the repository to make the example self-contained. This will also arguably make the python a bit cleaner.

What about something in between (which I'm halfway along, having added the .jpg to the PR): try to download the data, but, if necessary, catch the HTTPError for both the image and the categories, as we can direct users to the pre-downloaded files.

I quite like having some form of the fetch present, as, for example, I wanted to test a different dog, which only requires changing one line of code at the moment, and I think it makes an example better if the barrier to trying out different data is as low as possible.

Yes, testing alternative images did cross my mind, but the purpose of this example is to serve as an (ideally minimal) example of how to save a pytorch net and couple it to Fortran. To this end I'd maybe favour having the image in the repo to read from and removing the requests code, perhaps with a note in the Readme to describe how a user might go about trying other images.

[jatkinson1000]

@jatkinson1000 What sort of thing would you consider to be nicer? My feeling was that flattening it would make it the least ambiguous, or do you mean the way it is flattened/saved? (I'll add a comment about writing/reading anyway)

For the MiMA case we wrote out with the indices with the value (e.g. see here) to remove any ambiguity when reading back in and make interpretable.

[ElliottKasoar]

As discussed with @TomMelt and @jatkinson1000:

• Precision
  • While, in general, a separate precision module would be preferable, for this example it is probably clearest to keep the definition within the same file, but using c_wp rather than wp, to remove ambiguity with the commonly used Fortran-typed wp.
• Transposing
  • I added some discussion about when it is required, which hopefully clarifies things significantly
  • As suggested, I used reshape to simplify the operation in Fortran. As users are more likely to be familiar with Python reshaping/transposing than Fortran reshaping/transposing, I moved the transpose into the Python script, although I also added a note about how the alternative would be carried out
  • This complexity would also arguably be solved through the suggestion of storing indices alongside values, but I don't believe we concluded that there was a strong preference, so hopefully this is sufficiently clear as is

[ElliottKasoar]

At some point @TomMelt mentioned that it would be nice to have an assert to check that the results are actually consistent e.g. call assert_real_2d(big_array, big_result/2., test_name=msg) (currently unused) in the resnet benchmarking branch.

Given that the probabilities are more complex than that example, and seem to only match to ~5sf, is it worth just adding an assert for the label? Or maybe we should check the first 4-5 significant figures of the probability?

[jatkinson1000]

Given that the probabilities are more complex than that example, and seem to only match to ~5sf, is it worth just adding an assert for the label? Or maybe we should check the first 4-5 significant figures of the probability?

Yes, @ElliottKasoar, something like (checks relative error):

    ! Check error
    if (maxval(abs((gwfcng_x - gwfcng_x_ref)/gwfcng_x_ref)) >= 1.0e-6) then
      write(*,*) "AAARRGGHHHHH"
      stop
    endif

[jatkinson1000]

This is looking pretty good, nice work reading the categories etc. in to Fortran to make it a complete example!

Couple of comments as below.

Also, the python code could be more black, pylint, and mypy compliant.

[ElliottKasoar]

Also, the python code could be more black, pylint, and mypy compliant.

Was there anything obvious that you haven't mentioned? I'd already run it my main changes to resnet18.py through flake8 and black, and I think there were only a couple of minor things in resnet_infer_python.py, which I hadn't looked at enough yet (as part of the last set of changes, it's now updated to use the new image, as it was still using the ones.)

I've made a few more minor changes based on a few things from pylint/mypy too in the last commit, so hopefully it's ok now?

[TomMelt]

At some point @TomMelt mentioned that it would be nice to have an assert to check that the results are actually consistent e.g. call assert_real_2d(big_array, big_result/2., test_name=msg) (currently unused) in the resnet benchmarking branch.

Given that the probabilities are more complex than that example, and seem to only match to ~5sf, is it worth just adding an assert for the label? Or maybe we should check the first 4-5 significant figures of the probability?

The assert_real_2d subroutine has an optional argument which allows you to change the relative tolerance. So I think a good idea would be to use assert_real_2d (or something like it) but just lower the tolerance to acknowledge that the variable in question has higher inaccuracy.

snippet of assert_real_2d

  subroutine assert_real_2d(a, b, test_name, rtol_opt)

    implicit none

    character(len=*) :: test_name
    real, intent(in), dimension(:,:) :: a, b
    real, optional :: rtol_opt
    real :: relative_error, rtol

[jatkinson1000]

I'm happy with this now, and all running fine under checks for me.
I'll let @TomMelt take a look as well since it's been a large PR.

[TomMelt]

Looks great, just need to tidy up some fortran variable attributes

[ElliottKasoar]

Thanks @TomMelt! I think addressed all of your Fortran variable suggestions.

I also wanted to double check you were happy with the changes just before/at the same time as your review along side the assertion checks, which included the addition of wp separately to c_wp (the possibility of which was part of the intention for c_wp in the first place), since I'd changed probabilities to be a normal Fortran real.