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

Convert DataTree to Dataset and vice versa. #136

Closed
wants to merge 5 commits into from

Conversation

syedhamidali
Copy link
Contributor

  • Closes #xxxx
  • Tests added
  • Changes are documented in history.md

Building upon issue #133
I kindly request a thorough review of this pull request. It will alter the structure of DataTree (-> datatree.Cfradial2) and Dataset (-> xarray.Cfradial1), with some code duplication from backends to the transform modules. Your assistance in finalizing the better structure would be greatly appreciated.

Moreover, by structuring these functions within a class, it paves the way for future enhancements beyond conversion (e.g., dtree.to_cfradial1_dataset() or ds.to_cf2()) and exporting (ds.to_netcdf(), maintaining the cfradial1 data standard), including features like dtree.plot_ppi(). While this may be a potential future development, these changes currently encompass our modifications.

@syedhamidali syedhamidali self-assigned this Oct 23, 2023
@syedhamidali syedhamidali added the enhancement New feature or request label Oct 23, 2023
@syedhamidali syedhamidali modified the milestone: 0.4.0 Oct 23, 2023
@kmuehlbauer
Copy link
Collaborator

kmuehlbauer commented Oct 24, 2023

@syedhamidali First of all, thank you for taking on that difficult matter.

I'm a bit hesitant when it comes to subclassing. Xarray explicitely encourages downstream users to use composition over inheritance. And this aligns with my experience from the early wradlib xarray experiments.

Another aspect is that we might not be able to keep the requirements for being a CfRadial1/CfRadial2 class instance throughout it's lifetime. During processing relevant coordinates/variables/attributes might just get dropped or otherwise tampered with and subsequent calls to the conversion functions will fail. We would have to make sure that the class instance still contains what it should. I'm not sure we want to take on that burden to check every change in the class instance if we still have a valid CfRadial1/CfRadial2 object.

Instead I would favour keeping xarray.Dataset and datatree.DataTree and just implementing the conversion functions, which then could be accessed by accessors.

Update: That's only my POV. I'm not totally against using subclassing, if you or others have strong convincing arguments.

@syedhamidali
Copy link
Contributor Author

@kmuehlbauer Yeah, I completely agree with you, I am also hesitant to use custom subclassing, and I appreciate you sharing the helpful link. I've gained some initial insights into "extending xarray using accessors" from it. I'll explore this further, but if I run into any roadblocks, I may reach out to you or @mgrover1 for assistance. Thank you!

@syedhamidali
Copy link
Contributor Author

Closing this PR without merging, see #224

@syedhamidali syedhamidali deleted the transform branch October 30, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants