openradar / xradar

A tool to work in weather radar data in xarray
https://docs.openradarscience.org/projects/xradar
MIT License
85 stars 17 forks source link

Convert DataTree to Dataset and vice versa. #136

Open syedhamidali opened 8 months ago

syedhamidali commented 8 months ago

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.

kmuehlbauer commented 8 months ago

@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 commented 8 months ago

@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!