Download data from S3 bucket - #32
Conversation
juntyr
left a comment
juntyr
left a comment
There was a problem hiding this comment.
I like many of the changes and left some small suggestions.
My primary concerns is that some new code and comments seem to be AI generated, and if so I'd prefer another human pass to ensure that they don't sound so formulaic and over-engineered. Primarily-AI-generated files (or large parts) should also be marked as such since their authorship is more complicated
|
|
||
| To rebuild a dataset from its original data source instead, pass `--reprocess-data`: | ||
| ```bash | ||
| uv run python -m climatebenchpress.data_loader.datasets.cams --reprocess-data\ |
There was a problem hiding this comment.
Why the backslash at the end?
| "intake==0.7.0", | ||
| "intake-xarray==0.7.0", | ||
| "requests~=2.32.3", | ||
| "s3fs>=2024.10.0,<2025.4", |
There was a problem hiding this comment.
Why this specific range? The online lab might support a different version, so perhaps I'd need to patch/update this later
|
|
||
| def _differs(mine: object, yours: object) -> bool: | ||
| # NaN fill values are not equal to themselves, so compare them by repr. | ||
| return mine != yours and repr(mine) != repr(yours) |
There was a problem hiding this comment.
I'd use not ((mine == yours) or (np.isnan(mine) == np.isnan(yours))
| @@ -0,0 +1,201 @@ | |||
| """Check that the datasets published in the ClimateBenchPress object store are | |||
There was a problem hiding this comment.
This script feels AI generated. Is that the case? If so, I feel like there should be a comment saying who used AI and what AI you used. Maybe you can also do a human pass over the comments
| ) -> xr.Dataset: | ||
| """Download a given dataset and canonicalize it, i.e. ensure that all the axes names are consistent between different datasets. | ||
|
|
||
| By default, the pre-processed dataset is downloaded from the S3 ESIWACE |
| def open(download_path: Path) -> xr.Dataset: | ||
| return xr.open_zarr(download_path / "download.zarr").drop_encoding().chunk(-1) | ||
| ds = xr.open_zarr(download_path / "download.zarr").drop_encoding() | ||
| return ds.chunk(Cmip6Dataset.chunks(ds)) |
There was a problem hiding this comment.
Open could also be a classmethod to directly call cls.chunks
Since open now also has the contract to call ds.chunk and ds.drop_encoding, I wonder if we shouldn't split the method in two - We could have @Final open() in the abstract class that calls the abstract _open_impl() method in the sub-class; the open() method then applies the chunking and encoding before returning
|
|
||
| for v, da in ds.items(): | ||
| print(f"- {v}: {da.dims}") | ||
| main(CamsNitrogenDioxideDataset) |
| import xarray as xr | ||
| from climatebenchpress.data_loader import s3 | ||
|
|
||
| DEFAULT_REFERENCE = Path( |
There was a problem hiding this comment.
Maybe we don't hardcode the default path here?
|
|
||
| pbar.update(sizes[key]) | ||
|
|
||
| with ThreadPoolExecutor(max_workers=_NUM_WORKERS) as pool: |
There was a problem hiding this comment.
This will not work on Pyodide, we should add an explicit max_workers=0 path that runs everything in a normal loop
| @@ -0,0 +1,201 @@ | |||
| __all__ = [ | |||
There was a problem hiding this comment.
The comments also feel a bit AIy here
2 participants
This adds the functionality to download the datasets from the ESIWACE S3 bucket which for some datasets vastly reduces the dataset size that needs to be downloaded. We now also explicitly specify a chunking for each dataset and the downlad verifies that the dataset has the correct chunk sizes and, if not, rechunks the data. So if we just want to change the chunk size in the future we do not have to create separate datasets in the S3 bucket for that.