Skip to content
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

Implementing correct depth-to-sigma calculation #1772

Merged
merged 13 commits into from
Dec 5, 2024

Conversation

erikvansebille
Copy link
Member

This PR fixes the way that the conversion is done for CROCO fieldsets from particle.depth (in meters) to local sigma-coordinate. Where this was initially assumed to be a simple linear conversion, this turned out to be much more complex.

After some discussion on the myroms forum https://www.myroms.org/forum/viewtopic.php?p=25752#p25752, the best option is to calculate the local depth of each sigma layer and then linearly interpolate the particle depth to find the fractional sigma depth.

This fixes #1763

@erikvansebille erikvansebille marked this pull request as ready for review December 4, 2024 07:07
parcels/fieldset.py Outdated Show resolved Hide resolved
tests/test_advection.py Outdated Show resolved Hide resolved
parcels/field.py Show resolved Hide resolved
tests/test_advection.py Show resolved Hide resolved
@VeckoTheGecko
Copy link
Contributor

VeckoTheGecko commented Dec 4, 2024

Looks good! Just some things that I think would be good to clarify before merging.

I'm not 100% happy with the API changes with from_croco. I feel that having the hc= parameter is a deviation from the rest of the from_* functions. Though I don't have a better suggestion on how we could implement it given our limitations 🤔

erikvansebille and others added 3 commits December 5, 2024 08:09
@erikvansebille
Copy link
Member Author

I'm not 100% happy with the API changes with from_croco. I feel that having the hc= parameter is a deviation from the rest of the from_* functions. Though I don't have a better suggestion on how we could implement it given our limitations 🤔

Yes, I fully understand; I'm not 100% happy either.

This would actually be another reason to move to xarray for FieldSet creation in Parcels... The xarray DataSet can be expected to contain all these variables (hc, Cs_w, Zeta, H, etc) if the data comes from CROCO/Roms, so we can take them directly from there...

@VeckoTheGecko
Copy link
Contributor

This would actually be another reason to move to xarray for FieldSet creation in Parcels...

I 100% agree. I'm currently scoping that out

@erikvansebille erikvansebille merged commit 92548e1 into master Dec 5, 2024
16 checks passed
@erikvansebille erikvansebille deleted the CROCO_fix_sigma_calculation branch December 5, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

from_croco() enhancement; correct conversion from z to sigma grid
2 participants