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

[CANCELLED] Deprecate WRF diagnostic variables #140

Open
fmaussion opened this issue Jan 9, 2019 · 4 comments
Open

[CANCELLED] Deprecate WRF diagnostic variables #140

fmaussion opened this issue Jan 9, 2019 · 4 comments
Milestone

Comments

@fmaussion
Copy link
Owner

fmaussion commented Jan 9, 2019

It hurts me to do this because I spend a lot of time in designing an OOP API that allows to compute diagnostic variables out of WRF files which is quite elegant.

However, it's unlikely that we will ever have the time to support all possible diagnostic variables that wrf-python supports. Furthermore, wrf-python is faster because it relies on underlying fortran routines.

Therefore, we will start to deprecate WRF's diagnostic and vertical interpolation tools starting with salem v0.3.0 and focus our efforts on the salem - xarray bindings and the plotting capabilities.

@mullenkamp
Copy link

Hi,

I just discovered this package and was really glad to see that salem uses xarray/dask and assocaited syntax as close as reasonably possible. I was also glad to initially see that you were implementing diagnostic variables. I've checked out wrf-python as well, and although they use a lot of xarray under the hood, they don't use dask with xarray.
You state that wrf-python should be faster due to the underlying fortran routines, I've found that not only is wrf-python substantially slower when extracting single variables, it uses a rediculous about of RAM. Unless I'm missing something obvious.

Does your above statement mean that you will be getting rid of those diagnostic variables you already have or you will not be adding anymore?

Thanks!

@fmaussion
Copy link
Owner Author

fmaussion commented Feb 25, 2021

@mullenkamp thanks.

No, I don't plan to delete that code anymore. Salem has been on a low-key "maintenance mode" since a couple of years now and is still actively used and updated (for bugs, rarely for new features).

I am however unable to add more diagnostic variables because of lack of time. To be honest, I think that there is a great potential in some of the ideas I developed back in the days (it's a bit hacky but also elegant in a way).

At core, salem "tricks" xarray into believing that there are more variables in the netcdf files than there really is, by patching the netcdf objects before passing them to xarray: https://github.com/fmaussion/salem/blob/master/salem/sio.py#L969-L984 - it's really hacky but has proven quite stable until now. Adding new variables is relatively trivial. It's testing + maintenance that costs time.

Another idea worth exploring is to use dask delayed and add the lazy diagnostic variables to the xarray dataset after creation from the netcdf file, but I'm still unsure if that really works and is performant enough.

@mullenkamp
Copy link

Yeah, I totally understand with a lack of time to further develop it. I very much appreciate what you've already created and has given me ideas on how to handle these "diagnostic variables". It may seem "hacky" to add in these virtual variables, but it seems to work quite well with xarray and dask. As I mentioned, since I've tested the two options by salem and wrf-python, the salem solution seems optimal without having to reinvent anything substantial.
Thanks again for all the ideas! I'll put them to good use in New Zealand ;)

@sunt05
Copy link
Contributor

sunt05 commented May 23, 2021

Please don't simply deprecate this feature: I find it very useful and salem does provide a much more elegant approach than wrf-python.

Also, I can see the related code are very nicely structured, e.g., T2C here:

salem/salem/wrftools.py

Lines 180 to 193 in d3f2e5e

class T2C(FakeVariable):
def __init__(self, nc):
FakeVariable.__init__(self, nc)
self._copy_attrs_from(nc.variables['T2'])
self.units = 'C'
self.description = '2m Temperature'
@staticmethod
def can_do(nc):
return 'T2' in nc.variables
def __getitem__(self, item):
with ScaledVar(self.nc.variables['T2']) as var:
return var[item] - 273.15
)

So it should not be very hard to implement others.

Maybe better to set up a template for others to contribute procedures for these diagnostics?
Also, we may label this issue as help wanted to attract folks' attention and contributions.

@fmaussion fmaussion changed the title Deprecate WRF diagnostic variables [CANCELLED] Deprecate WRF diagnostic variables Nov 2, 2022
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

No branches or pull requests

3 participants