introducing wave forcing for sediment transport and tidal inlet test case

[seimurss]

Wave forcing was introduced for the radiation stresses following fragkouan/thetis_wci (Fragkou et al., 2024) and similarly, other fields, such as Hs, Ub, T, wave dir, and freq. were introduced and adapted in sediment transport model, albeit only orbital velocities are accounted for in bottom stresses computation.
Tidal inlet test case (Warner et al., 2008) was developed to test the seabed evolution in the presence of waves and tides.

[stephankramer]

This looks pretty good already!

My main feedback/questions at this moment are:

• what is the provenance of the data in the example. There are two .msh files do they have corresponding .geo files? One of these contains bathymetry data, where does that data come from? Is it interpolated from some netcdf file? Or is it based on an idealised analytical expression. The preferable option would be to just have a .geo file for the 2D geometry (and the output .msh) and either recreate the bathymetry on the fly at run-time, or if it is interpolated from some dataset that we can't commit to the repo, commit a firedrake checkpoint file with the interpolated data with clear instructions (a script) how to obtain the required data set and regenerate that checkpoint file (so we are able to redo this in the future when any of the file formats change). Then there's also a /Users/sshirinov/mac/cmcc/shyfem/cases/inlet/tideinlet3/ww3waves.nc where does that come from?
• Add a little more description at the head of the example to explain what the example consists of, what functionality it introduces, and a more complete reference: Warner et al. (2008) is ambiguous, maybe just add the doi link: https://doi.org/10.1016/j.cageo.2008.02.012
• at the end of thetis/solver2d.py there seem to be two new timeloops that look like they've been copied from the existing one - presumably with changes applied to it, but it's hard to see what the changes are. These should be integrated into the existing timeloop, with some logic as to when these required changes would apply.

Assuming these can be addressed, have another look through the diff yourself and make sure there's no changes you didn't mean to include (in particular don't include commented out code). Then if you remove the draft status, I would be happy to go through it in more detail line-by-line, with some nitty-gritty things (none of it really interesting). At a first glance I couldn't see any major issues implementation wise.

[seimurss]

Update: having merged latest changes from the main repo to the fork (along with some modifications following Stephan's comments), and having re-compiled petsc-firedrake-thetis with their latest release versions, i ended up with an issue running pytests due to the line in utility3d.py: self.solver.mesh.clear_rtree(). Replacing it with clear_spatial_index() allows all tests to pass.

@cpjordan, mentioning this here in case you've come across something similar before.

[stephankramer]

This depends on whether you're using Firedrake release or Firedrake main. If you're using Firedrake release (or a slightly older Firedrake main) it is clear_spatial_index indeed, with Firedrake main (the development branch) it's clear_rtree. However since you're planning to merge this into Thetis main, you should really be testing with a Firedrake main installation, following instructions here: https://www.firedrakeproject.org/firedrake/install.html#id38

[stephankramer]

Of course you can also just see whether things are failing on the CI.
So currently there's two minor things:

• you appear to use pyvista in the example, which is fine but that means it needs to be added as a requirement here i.e. in the docs requirement. We should probably have a separate "demos" target but currently e.g. matplotlib (which you also really need in quite a few demos/examples) is also listed under there - so let's do that in another PR
• you're failing the lint test (python style guide rules). You should ideally run "make lint" for every commit

[stephankramer]

I've also invited you to the Thetis developer team. This has the advantage that you can push your branch (under a suitable name) to this repo. If you then change the source branch in this PR to that branch, the CI will run automatically every time you push. Currently we have to give permission first for it to run...

[seimurss]

This depends on whether you're using Firedrake release or Firedrake main. If you're using Firedrake release (or a slightly older Firedrake main) it is clear_spatial_index indeed, with Firedrake main (the development branch) it's clear_rtree. However since you're planning to merge this into Thetis main, you should really be testing with a Firedrake main installation, following instructions here: https://www.firedrakeproject.org/firedrake/install.html#id38

This seems to have solved the problem, thank you Stephan!
I am yet to revise the visualization script, and will see if i could avoid the use of packages not included in the optional dependencies. There is also xarray and meshio in the running script, will test if those could be avoided, alternatively add the them to the list as you suggested.

Thanks for the invite, it should make things much easier then. i'll see how to reroute the source branch of the PR and test it as i complete some sensitivity analysis with the experiment.

[seimurss]

Pausing this PR, as it cannot be re-linked to a new branch as a source branch (fork -> new branch within repo).
So, the changes have been integrated into the tidalinlet-wave-forcing branch in the upstream (thetisproject), along with some fixes and additions. Once ready will raise a new PR from there, and close this one.
The wave data generation and instructions file will be added there in subsequent commits