Skip to content

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