-
Notifications
You must be signed in to change notification settings - Fork 48
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
Allow basic xarray commands to be applied to whole PredictionEnsemble object #102
Comments
I forgot to mention one thing during the call that I'm worried about the maintainability of building an object over xarray datasets. From https://xarray.pydata.org/en/stable/internals.html " The standard advice is to use composition over inheritance, but reimplementing an API as large as xarray’s on your own objects can be an onerous task, even if most methods are only forwarding to xarray implementations. Like just recently, xarray introduced the coarsen method which may be useful for this project (as well as interp method and roll method). A suggestion thru accessors; now you can use the native xarray methods such as filtering by attrs. The naming convention and dimensions should definitely be checked in the function, but this is just a rough sketch Then you can implement a score method: I understand that I'm introducing new things and Aaron said not to :P so you guys can decide!
|
I had strong objections about introducing such things after two weeks. |
About the user interface: I prefer to just have one data variable in the process so you can also compute skill over a dataset with lots of variables. That may be a particular use case for the perfect model only. Accessors sound useful. You probably know more about them than We do. |
Thanks for bringing up this discussion. I see your point here about missing out on new features/it breaking by not using accessors. I think I'm concerned by a the following things with this accessor mockup you did:
On the bright side:
With the classes, I really appreciate that we can get a clear view of what's going on: If the biggest concern is to allow for multiple models, I can modify the classes to accept them. @ahuang11, let me know your thoughts on all this. I'm open to overhauling these classes for the next round (v2), so we can have plenty of time to discuss this. I think accessors might limit the scope of our project for the above reasons. But they also lend important features like keeping up with new |
Yep you can do
Or write a separate function completely
and in the class, check if it's labeled properly.
Right, it was a quick mockup. You could add any arbitrary attributes to the dataset, and use filter_by_attrs or do a simple
No comment :p
You don't have to. You can instantiate the instance once, cp = ClimPred(ds) and then do cp.function().
You don't have to. You can again add variable to attributes and filter that way.
Sure, this was just a mockup. You can easily return stat_ds.
Yes, that is a good overview, and I believe you can reimplement that as an accessor method like
Yes, v2 sounds like a good target. I suppose I want to get this right ahead of time so it won't be like a Python 2 to 3 transition if this package gets popular :P |
For xr commands like .groupby('time.year').mean('time') or coarsen() I dont see any differences between perfect-model and hindcast type. |
@ahuang11 -- thanks for the clarifications. I figured there were alleviations for all points but wanted to see what you had to say. I think accessor is the way to go, and maybe that's the major focus for the v2 rollout that you and I can focus on. I am particularly happy to hear we can rewrite as cp = ClimPred(ds) for instance.
I agree 100%. This forced me to learn python classes anyhow so I'm not mad. Let's put a pin in this for now and pick it up in a conference call following v1's completion. For now, we can have the classes access our updated functions (which shouldn't require change) and then will do a rewrite "early on" to switch to accessors.
Agreed. My comment was more about comparisons and so on. But we can alleviate that by forcing a Good conversation! Excited to dig into this. Appropriately implementing this will put the package in the place I really want to see it... with smart easy-to-use objects and methods. |
I think it would be extremely useful to be able to call something like
ReferenceEnsemble.sel(time=slice(1980, 2000))
and have it apply to every product (e.g., reconstruction, data, uninitialized run).Some commands I'd like to implement:
sel()
isel()
squeeze()
groupyby()
rolling()
resample()
The text was updated successfully, but these errors were encountered: