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

Creating from_xarray_delft3d methods #1331

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

vesnaber
Copy link

@vesnaber vesnaber commented Mar 6, 2023

This is a PR to create a FieldSet.from_xarray_delft3d method. Will be testing it before merging it

@VeckoTheGecko
Copy link
Contributor

VeckoTheGecko commented Mar 6, 2023

Hey @vesnaber , not sure how far along you are in with your implementation, but I'll point to my branch in #1228 which has a rudimentary implementation which might help.

My work on that PR grew stale as other areas in the codebase took my interest. Happy for you to take lead and I can help in any way you see fit 😁 (eg. reviewing PRs, stress-testing implementations etc.)

EDIT: "rudimentary" meaning that it didn't use the parcels API, and was capped to certain mesh geometries. Basically the minimum requirements to do the work that I was wanting to 😅

@vesnaber
Copy link
Author

vesnaber commented Mar 6, 2023

Hey @vesnaber , not sure how far along you are in with your implementation, but I'll point to my branch in #1228 which has a rudimentary implementation which might help.

My work on that PR grew stale as other areas in the codebase took my interest. Happy for you to take lead and I can help in any way you see fit 😁 (eg. reviewing PRs, stress-testing implementations etc.)

EDIT: "rudimentary" meaning that it didn't use the parcels API, and was capped to certain mesh geometries. Basically the minimum requirements to do the work that I was wanting to 😅

Hey @VeckoTheGecko thank you for pointing me to your branch - I will check it out in detail and try to implement it to the parcels API! My application at the moment is non-rotational equidistant rectangular grid. So far I have been changing the Delft3D mesh output locally (land masks from NaNs to lon/lat) but I will soon make it a part of the FieldSet function. The next step will be to implement it to different grid geometries.

Thank you for your offer to help! It is great to see that more people are interested in D3D-Parcels implementation. Will surely contact you about the progress/reviewing! 😁

@VeckoTheGecko
Copy link
Contributor

My application at the moment is non-rotational equidistant rectangular grid... The next step will be to implement it to different grid geometries.

Looks like we have the same implementation! I was only working in a rectangular grid as well. I think quite a lot of nuance (which I wasn't able to dig much in to) is introduced from supporting arbitrary geometries due to the ways D3D and parcels deal with grids.

Will surely contact you about the progress/reviewing! 😁

Sounds good to me. Let me know, I'll be tuned into this PR 🙂

@vesnaber
Copy link
Author

My application at the moment is non-rotational equidistant rectangular grid... The next step will be to implement it to different grid geometries.

Looks like we have the same implementation! I was only working in a rectangular grid as well. I think quite a lot of nuance (which I wasn't able to dig much in to) is introduced from supporting arbitrary geometries due to the ways D3D and parcels deal with grids.

Will surely contact you about the progress/reviewing! 😁

Sounds good to me. Let me know, I'll be tuned into this PR 🙂

I just implemented a few lines from your branch - the ones that deal with getting D3D output to 'look like' mitgcm indexing. With this change I was able to run Parcels with my D3D output (rectangular grid - both non-rotational and rotational, with depth averaged flow fields) without changing the grid. I left the land values as 0 (zeros) like they are originally in D3D and I got no issues so far, so maybe this code could even work with different geometries - we'll see!

At the moment I don't have any D3D outputs with other grid geometries ready, so it might take some time before I continue with testing.:)

@VeckoTheGecko
Copy link
Contributor

VeckoTheGecko commented Mar 12, 2023

Sounds good. I'm quite busy this coming week, so won't really be able to take a closer look. Happy to put some time aside next weekend to provide feedback.

EDIT: Sorry that I didn't get around to this (other parts of the codebase grabbed my attention). I'll see if I can sit down and take a dive into this in the near future (no promises unfortunately though 😢 )

@VeckoTheGecko
Copy link
Contributor

Since we have a near-working implementation of this for a cartesian grid, I think it would be great to slowly* work towards getting this incorporated into the main package. Even if the coverage for different grid geometries is limited at the moment, I think it will be beneficial to the community.

Things needing doing:

  • Resync with master
  • Writing unit tests and correcting implementation along the way
  • Writing documentation

Re. the documentation, I think giving some good code examples in the docstring is sufficient. I don't think its worth creating a dedicated notebook (and I also can't be bothered doing so 😅). We can shout out the method in another notebook if we want to bring attention to it

@vesnaber would it be alright if I pushed to this branch? Or should I open another PR?

cc @erikvansebille

*I have quite a few projects going on at the moment, and am away Jan-Jun 2024, so it will be slow from my side :)

@vesnaber
Copy link
Author

Since we have a near-working implementation of this for a cartesian grid, I think it would be great to slowly* work towards getting this incorporated into the main package. Even if the coverage for different grid geometries is limited at the moment, I think it will be beneficial to the community.

Things needing doing:

  • Resync with master
  • Writing unit tests and correcting implementation along the way
  • Writing documentation

Re. the documentation, I think giving some good code examples in the docstring is sufficient. I don't think its worth creating a dedicated notebook (and I also can't be bothered doing so 😅). We can shout out the method in another notebook if we want to bring attention to it

@vesnaber would it be alright if I pushed to this branch? Or should I open another PR?

cc @erikvansebille

*I have quite a few projects going on at the moment, and am away Jan-Jun 2024, so it will be slow from my side :)

@VeckoTheGecko My apologies for the late reply! Unfortunately, I put this PR on hold as I switched to another model, but I hear quite some interests from other users to push this PR with the current developments. So it will indeed be very beneficial to getting it incorporated into the main package!

I'm completely fine if you push it to this branch. I am also absent quite a lot in the upcoming months, but after March I can also work on one of the tasks (e.g. documentation).

@VeckoTheGecko
Copy link
Contributor

I'm currently engaged on a different project, so I don't know my capacity to dedicate time to this (especially since I've had to remove my Delft installation for technical reasons). I'm travelling from end Jan to end Jun 2024 without a laptop, so that further complicates things.

If someone else wants to contribute to this PR please comment.

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.

None yet

2 participants