Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

atrisovic
Copy link
Contributor

@atrisovic atrisovic commented Sep 18, 2019

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

Copy link
Member

@delgadom delgadom left a 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
Copy link
Member

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 :)

Copy link
Contributor Author

@atrisovic atrisovic Sep 19, 2019

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?

Copy link
Member

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.

@atrisovic
Copy link
Contributor Author

I have a few proposed changes that we could discuss though... I'll document them and create a PR.

@delgadom if it's not a major change you can leave a comment and I'll fix it

@delgadom
Copy link
Member

Unfortunately it's a large number of changes scattered across climate_toolbox and climate_transform_specs. I should have warned you that I had started work but didn't complete it. My current proposal is on the standardized_runner_func branch. I'll file a WIP PR so we can discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants