Adds Translators to xarray_wrappers.py #594
Conversation
There was a problem hiding this comment.
Looks great, @NVJY! I left some comments on suggested naming (for consistency with AutoMol) and suggested some ways to simplify the API by combining closely related functions. I will go ahead and merge the PR and you can address these things in a separate PR as you have time.
| @@ -18,75 +17,106 @@ def from_data(temps, press, rates): | |||
|
|
|||
| # Getters | |||
| def get_pressures(ktp): | |||
There was a problem hiding this comment.
I have generally followed the practice of omitting the "get" prefix on functions and just naming it after the return value. So, I would prefer just to call these functions pressures(), temperatures(), etc.
There was a problem hiding this comment.
(I do add other verbs, though, such as "set", when the function isn't simply retrieving a value.)
| """ | ||
| Construct a KTP DataArray from data | ||
| """ | ||
| """Construct a KTP DataArray from data""" | ||
|
|
||
| ktp = xarray.DataArray(rates, (("pres", press), ("temp", temps))) |
There was a problem hiding this comment.
It would be good here to first cast the values to specific datatypes for consistency:
rates = numpy.array(rates, dtype=float)
press = numpy.array(press, dtype=float)
temps = numpy.array(temps, dtype=float)
| """ | ||
| Gets the temperature values | ||
| """ | ||
| """Gets the temperature values""" | ||
|
|
||
| return ktp.temp.data | ||
|
|
||
|
|
||
| def get_values(ktp): |
There was a problem hiding this comment.
This function and the next three could be combined into a single, generic function for accessing values:
def values(ktp, pres=None, temp=None):
if pres is None and temp is None:
return ktp.values
elif temp is None:
return ktp.sel(pres=pres)
elif pres is None:
return ktp.sel(temp=temp)
return ktp.sel(temp=temp, pres=pres)
I have found this to be generally more convenient (and easier from a user perspective) than defining separate functions for closely related things
| """ | ||
| Get a specific value at a selected temperature and pressure value | ||
| """ | ||
| """Get a specific value at a selected temperature and pressure value""" | ||
|
|
||
| return ktp.sel(temp=it, pres=ip) | ||
|
|
||
|
|
||
| def get_ipslice(ktp, ip): |
There was a problem hiding this comment.
As with the previous set, I think this and the next could be combined into a single def islice(ktp, ip=None, it=None) like the above, except calling isel() methods.
| """ | ||
| Sets the KTP values | ||
| """ | ||
| """Sets the KTP values""" | ||
|
|
||
| ktp.loc[{"pres": pres, "temp": temp}] = rates | ||
| return ktp |
There was a problem hiding this comment.
I would pass this back through your from_data function above before returning:
return from_data(
temps=temperatures(ktp), # currently called `get_temperatures()`
press=pressures(ktp), # currently called `get_pressures()`
rates=values(ktp) # currently called `get_values()`
)
I have found it very convenient to use from_data as a central function for validating and normalizing data, so that, if I ever need to make a change to those things, the change is concentrated in one function rather than distributed across the code.
|
|
||
| # Translators | ||
|
|
||
| def dict_from_xarray(xarray_in): |
There was a problem hiding this comment.
I would change this function signature to the following:
def dict_(ktp)
since xarray_in is just a ktp like all the other functions, and from_xarray is implied (since all of the functions act on a ktp xarrays).
There was a problem hiding this comment.
(I would add the from_x qualifier if, for whatever reason, I need to include a function here that didn't act on a ktp)
|
|
||
| #Stopped working on this one because it was less critical! | ||
|
|
||
| #def xarray_from_dict(ktp_dct): |
There was a problem hiding this comment.
When ready, this function can just be called
def from_dict(ktp_dct)
for consistency with from_data() and any other functions you may add eventually (e.g. from_string()). Since the whole module concerns ktp xarrays, there is no need to specify that you are getting an xarray.
There was a problem hiding this comment.
As above, I would add the qualifier if the function was returning something other than a ktp.
A translator that takes an xarray and changes it into a ktp_dct item was added to xarray_wrappers.py, and a test was added to xarray_wrappers_test.py as well.
A commented translator that takes a ktp_dct and turns it into an xarray is also there, but it is nonfunctional currently.