Made new xarray context behavior to register multiple tables from a dataset#166
Conversation
alxmrs
left a comment
alxmrs
left a comment
There was a problem hiding this comment.
Looking really good! Here are my thoughts on design.
| return self | ||
|
|
||
|
|
||
| def _group_vars_by_dims(ds: xr.Dataset) -> dict[tuple[str, ...], list[str]]: |
There was a problem hiding this comment.
I'd like to see a few unit tests for this method, even though it is private.
|
|
||
| return self | ||
|
|
||
| def from_dataset_multi( |
There was a problem hiding this comment.
High level feedback: I think we should roll this into from_dataset and it should be the "default" behavior. Since we are an experimental project, I'm OK with changing the public API.
I think @ghostiee-11 or @ahuang11 might have a few ideas about what good names would be for these kwargs, i.e. what is most conventional for Xarray. So far, these LGTM.
|
|
||
| def from_dataset_multi( | ||
| self, | ||
| dataset_name: str, |
| self, | ||
| dataset_name: str, | ||
| input_table: xr.Dataset, | ||
| dim_aliases: dict[tuple[str, ...], str] | None = None, |
There was a problem hiding this comment.
I think should be a keyword only argument, see: https://python-3-for-scientists.readthedocs.io/en/latest/python3_advanced.html
There was a problem hiding this comment.
I like "aliases", but "dim" is a bit unclear; I'm not sure exactly what these should be called. Maybe "dim_group_aliases"? I'm curious what @ahuang11 and @ghostiee-11 think, too.
| dataset_name: A name for the dataset, used as a prefix for all | ||
| registered table names. | ||
| input_table: An xarray Dataset. Variables may have differing dimensions. | ||
| dim_aliases: Optional mapping from dimension tuples to custom table |
There was a problem hiding this comment.
I like that we have an aliases override.
| suffix = dim_aliases.get(dims, "_".join(dims)) | ||
| table_name = f"{dataset_name}_{suffix}" | ||
| sub_ds = input_table[var_names] | ||
| self.from_dataset(table_name, sub_ds, chunks) |
There was a problem hiding this comment.
I like this pattern, so much so that I think in the new version that we could turn the existing from_dataset into a private method (_from_dataset). Then, in a method like this, we could have a big if or return early logic to tell if there is more than one group of dims.
2 participants
This should account for the differing dimensions between variables