-
Notifications
You must be signed in to change notification settings - Fork 3
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
add data loaders #29
base: master
Are you sure you want to change the base?
add data loaders #29
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @atrisovic - this is a great start. I'm really sorry - my claim that I could hammer something out in a couple hours proved to be too much for my schedule and I spent about an hour on this and then got pulled into other things. I have a few proposed changes that we could discuss though... I'll document them and create a PR.
ds.load() | ||
raise TypeError("'" + data_type + "' not supported. Supported data " | ||
"types are: NASA BCSD, GMFD, BEST, ERA5.") | ||
return loader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is great! but can we force the user to provide the exact loader name? ERAI and ERA5 have different formats, and GMFD-v1 and GMFD-v3 do too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my first implementation was for the users to pass the loader function, but that would mean that a user would need to import both the loader function and load_climate_data (from climate_toolbox.io.io import load_climate_data, load_bcsd
) to be able to pass it on. but yeah, I can actually revert it back to that version, it's good if a user wants to write it's own loader func. what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry I didn't say that quite right. I don't mean load_era
, I mean something like ERA5
rather than ERA
. Different versions have different formats.
@delgadom if it's not a major change you can leave a comment and I'll fix it |
Unfortunately it's a large number of changes scattered across |
Data loaders
(1) (reformatted) NASA_BCSD
Tasmax, Tasmin, Tas: no changes when loading
(2) GMFD
TMIN, TMAX: rename_coords_to_lon_and_lat + convert_lons_split
TAS: convert_lons_split