Skip to content

Test FTorch integration#47

Draft
ElliottKasoar wants to merge 11 commits intomainfrom
ftorch
Draft

Test FTorch integration#47
ElliottKasoar wants to merge 11 commits intomainfrom
ftorch

Conversation

@ElliottKasoar
Copy link
Collaborator

@ElliottKasoar ElliottKasoar commented Dec 14, 2023

  • Currently based off cam-interface, this primarily creates a new pair of Fortran files that mirror nn_cf_net, nn_convection flux , integrating the PyTorch implementation of the neural network via FTorch.

  • test.f90 has also been updated to call each function from the original and new (..._torch.f90) files, checking that the outputs match for a dummy input.

  • More work could be done on the Python implementation, but pt2ts.py, based on the FTorch utils version, has been used to save the model, with the main addition being functionality to read the current netCDF file used to store model weights and convert this to a pickled file.

    • Currently this netCDF file is still required for mean/variance, but this could be fixed in a similar manner to cgdrag, where they are instead attached to the saved model.
  • Also includes a starting point for cmake/make files (i.e. both build for me, although potentially requiring different input file paths), to allow the tests to link to the FTorch and netCDF libraries.

@ElliottKasoar ElliottKasoar changed the title Ftorch Test FTorch integration Dec 14, 2023
@casterkay casterkay force-pushed the ftorch branch 2 times, most recently from db56049 to 179eb3f Compare March 18, 2024 16:23
@jatkinson1000
Copy link
Member

Hi @tztsai ad0be72 seems to revert a few of the changes you made as part of #53 ?

Also, whilst sometimes necessary I would caution against getting into the habit of regular force-pushing, and also encourage you to use --force-with-lease if you have to.

I wonder if it is worth starting a new PR inspired by this one (or using cherry-picks from this branch) as @ElliottKasoar branched this off of #44 which has changed a lot since. and resolving all the conflicts could get very messy.

@casterkay
Copy link
Contributor

Hi @tztsai ad0be72 seems to revert a few of the changes you made as part of #53 ?

Also, whilst sometimes necessary I would caution against getting into the habit of regular force-pushing, and also encourage you to use --force-with-lease if you have to.

I wonder if it is worth starting a new PR inspired by this one (or using cherry-picks from this branch) as @ElliottKasoar branched this off of #44 which has changed a lot since. and resolving all the conflicts could get very messy.

There was some syntax error in test.f90 so I tried to resolve it. I went a bit back and forth so I forced pushed to overwrite some unnecessary changes, all of which were made by myself. Thanks for the tip about --force-with-lease, I'll use it afterwards.

@jatkinson1000
Copy link
Member

@tztsai a lot of the conflicts here are in files that do not directly relate to this PR. (e.g. in CAM_mods)

  • Add pt2ts.py
  • Add nn_cf_net_torch.f90
  • Add nn_convection_flux_torch.f90
  • Add CMake build process for FTorch components
  • Add testing for FTorch implementation matching that existing for the current Fortran net

I think if you look at Elliott's commits (not mine) from the PR you'll see this is what they cover.
Branching off of cam-interface (you could try rebasing on this if you really wanted) should give you a reasonably stable version of nn_convection_flux.f90 and nn_cf_net.f90 on which to base things.

Let me know if you want to discuss this in more detail.

@jatkinson1000
Copy link
Member

I see you are targeting cam-interface so would suggest you try rebasing off of that.
If that is too messy then perhaps a new branch from cam-interface could cherry-pick the commits made by Elliot on this branch? (e562143 to fd57b2f).

I have already merged your updates to main from #53 into the cam-interface branch.

@casterkay casterkay changed the base branch from cam-interface to main March 18, 2024 18:40
@casterkay
Copy link
Contributor

casterkay commented Mar 18, 2024

@tztsai a lot of the conflicts here are in files that do not directly relate to this PR. (e.g. in CAM_mods)

  • Add pt2ts.py
  • Add nn_cf_net_torch.f90
  • Add nn_convection_flux_torch.f90
  • Add CMake build process for FTorch components
  • Add testing for FTorch implementation matching that existing for the current Fortran net

I think if you look at Elliott's commits (not mine) from the PR you'll see this is what they cover. Branching off of cam-interface (you could try rebasing on this if you really wanted) should give you a reasonably stable version of nn_convection_flux.f90 and nn_cf_net.f90 on which to base things.

Let me know if you want to discuss this in more detail.

I didn't choose cam-interface as the target branch to merge into, but I have changed the base branch to main now. As you said, the conflicts have disappeared now.

@jatkinson1000
Copy link
Member

I didn't choose cam-interface as the target branch to merge into, but I have changed the base branch to main now. As you said, the conflicts have disappeared now.

I didn't suggest targeting main, sorry for any confusion.
Bear in mind that this PR references files that are part of cam-interface (and contains commits from cam-interface) and not main (hence why the conflicts disappeared).
As such these will re-emerge once cam-interface is merged.
On top of that the files I list above that need adapting to use FTorch (the entire point of this PR) are part of the cam-interface branch, so you need to be working with the latest versions from there.
This is why I suggest rebasing on cam-interface if possible, and creating a new branch perhaps cherry-picking if not.

Let's meet to discuss this at some point this week so we can clear things up.
Perhaps Wednesday morning (or tomorrow but I am less sure of my availability).

@jatkinson1000
Copy link
Member

@tztsai please could you pause work on this for now.
I see you have removed many commits and forced again.
I think we need to sit down and discuss this before going further so that we are both clear what the objective of this PR is, have a clear plan, and know how best to proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants