-
Notifications
You must be signed in to change notification settings - Fork 168
Create convert.croco_to_sgrid function #2481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
As the v3 interpolation turns out to be wrong too
Since we don't need to convert to C anymore, the return value also doesn't have to be a np.float32
Using XLinear interpolation gives gives much better results for the tutorial (particles stay closer to initial depth). This was also done in v3
docs/user_guide/v4-migration.md
Outdated
| - `math` functions should be replaced with array compatible equivalents (e.g., `math.sin` -> `np.sin`). Instead of `ParcelsRandom` you should use numpy's random functions. | ||
| - `particle.depth` has been changed to `particles.z` to be consistent with the [CF conventions for trajectory data](https://cfconventions.org/cf-conventions/cf-conventions.html#trajectory-data), and to make Parcels also generalizable to atmospheric contexts. | ||
| - The `InteractionKernel` class has been removed. Since normal Kernels now have access to _all_ particles, particle-particle interaction can be performed within normal Kernels. | ||
| - Users need to explicitly use `convert_z_to_sigma_croco` in sampling kernels such as the `AdvectionRK4_3D_CROCO` kernel for CROCO fields, as the automatic conversion from depth to sigma grids under the hood has been removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Users need to explicitly use `convert_z_to_sigma_croco` in sampling kernels such as the `AdvectionRK4_3D_CROCO` kernel for CROCO fields, as the automatic conversion from depth to sigma grids under the hood has been removed. | |
| - Users need to explicitly use the `convert_z_to_sigma_croco` kernel within sampling kernels (such as the `AdvectionRK4_3D_CROCO` kernel) when working with CROCO fields, as the automatic conversion from depth to sigma grids under the hood has been removed. |
This using of a kernel inside a kernel seems quite novel to me - I assume this wasn't possible in version 3? Are there any other usecases where we can highlight this (i.e., stuff that users can now do that wasn't possible with sequential kernels?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 74a95ca
Note that the convert_z_to_sigma_croco is not a kernel, but a helper function, so we shouldn't call it a Kernel. And yes, this was not possible in JIT, but is now possible in v4 (was always possible in Scipy/v3). We have other helper functions like _constrain_dt_to_within_time_interval() too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the
convert_z_to_sigma_crocois not a kernel, but a helper function
Gotcha, I misread the function signature (I saw "fieldset" in it and assumed the rest was the same)
For these sort of functions, should we vendor them out of parcels.kernels? Perhaps we can for this one - but if we know we are going to have more then maybe a different namespace would be better
This PR supersedes #2450 by creating a
convert.croco_to_sgrid()function, and using that to support croco