OpenDrift / opendrift

Open source framework for ocean trajectory modelling
https://opendrift.github.io
GNU General Public License v2.0
232 stars 113 forks source link

LarvalFish model requires more variables for Stokes drift? #1203

Closed kthyng closed 6 months ago

kthyng commented 6 months ago

Hi! I'm setting up for a few different models. It looks like LarvalFish is meant to be usable with Stokes drift, but when I try to use it I hit an error because the Stokes drift x/y velocity variables are not set as required in the model. It seems that I am able to get this to work by adding to the LarvalFish required variable list:

        'sea_surface_wave_stokes_drift_x_velocity': {'fallback': 0},
        'sea_surface_wave_stokes_drift_y_velocity': {'fallback': 0},

but I wanted to be sure this is an appropriate change. Is there a reason they aren't already there or just an edge case that hasn't been used? Or is Stokes drift not meant to be used with LarvalFish?

knutfrode commented 6 months ago

Hi,

LarvalFish is a fairly basic demonstrator module, and Stokes drift has not been added to it yet. But the Stokes drift functionality inherited from OceanDrift (actual implementation is in PhysicsMethods, see below) will be applicable to LarvalFish if you: 1) Add stokes variables to required_variables like you already did. 2) Add the call self.stokes_drift() to the update method.

If you use loglevel=0 (generally recommended), you can verify that the log shows that Stokes drift is in fact applied.

The actual implementation of Stokes drift is found in the PhysicsMethod class: https://opendrift.github.io/_modules/opendrift/models/physics_methods.html#PhysicsMethods.stokes_drift

You are welcome to send a pull request with this update, if you like. Otherwise I can do it.

kthyng commented 6 months ago

Ok I can put in a PR for that. Do you have a test you'd like implemented?

I see that wind drift isn't implemented — would that be worth adding in? I don't know enough about larvae movement in particular to know if they should be impacted by it.

knutfrode commented 6 months ago

I believe direct wind drift should not be necessary for larvae. Ekman current should already be included in ocean model output, and wave motion is represented by Stokes drift. The additoinal wind drift is in OpenDrift (as in most other drift models) represented by a wind_drift_factor times the wind, applying to elements/objects at the very surface (possibly sticking into air), but larvae shold not spend much/any time at the very surface.

There is no tests yet for the LarvalFish class, only a placeholder: https://github.com/OpenDrift/opendrift/blob/master/tests/models/test_models.py#L178

Adding a test is not critical here, as the module is not used for operational purposes (unlike e.g. OpenOil and Leeway), and general features like Stokes drift etc have their own dedicated tests. Also the tests (numbers) would have to be updated each time the LarvalFish module is updated. But you are welcome to add a small test if you like; only criterion is that it should be pretty fast, ideally less than ~3 seconds, as there are already more than 200 unit tests in OpenDrift.

kthyng commented 6 months ago

Thank you!