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 columns_with_iterables parameter to esm_datastore #589

Merged
merged 11 commits into from Mar 30, 2023

Conversation

dougiesquire
Copy link
Contributor

@dougiesquire dougiesquire commented Mar 29, 2023

This PR adds an optional columns_with_iterables parameter to the intake_esm.esm_datastore API that specifies the columns to convert with ast.literal_eval. This enables intake yaml descriptions of intake-esm catalogs with multi-variable assets that work by default. See #587 for context/motivation.

Note, I also explored using intake dataset transforms, but I don't think this provides quite the functionality we need.

Fixes #587

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable

@dougiesquire dougiesquire changed the title add columns_with_iterables parameter Add columns_with_iterables parameter to esm_datastore Mar 29, 2023
Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach, especially with the error catching here. Would you be willing to add an example as well? So the docs reflect these new changes?

@mgrover1
Copy link
Collaborator

Also a test here would be helpful

@dougiesquire
Copy link
Contributor Author

Thanks @mgrover1. Tests and docs added. Note, I had to pin netcdf4<1.6.0 to get the tests to pass and docs to build. This is due to a change in 1.6.1 that seems to be causing issues all over the place, see e.g. Unidata/netcdf4-python#1192

Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor changes - I appreciate the contribution @dougiesquire !! Great work!

dougiesquire and others added 3 commits March 30, 2023 12:54
Co-authored-by: Max Grover <mgroverwx@gmail.com>
Co-authored-by: Max Grover <mgroverwx@gmail.com>
Co-authored-by: Max Grover <mgroverwx@gmail.com>
@mgrover1 mgrover1 merged commit 4fb920c into intake:main Mar 30, 2023
6 checks passed
@andersy005
Copy link
Member

thank you for this addition, @dougiesquire & @mgrover1!

@dcherian
Copy link
Collaborator

Should this info be stored in the JSON instead? It's a property of the catalog, so the information can be specified at write-time instead of at read-time.

@andersy005
Copy link
Member

Should this info be stored in the JSON instead? It's a property of the catalog, so the information can be specified at write-time instead of at read-time.

i'm definitely in favor of supporting this when it's defined in the JSON and adding it to the spec: https://github.com/intake/intake-esm/blob/main/docs/source/reference/esm-catalog-spec.md

@mgrover1
Copy link
Collaborator

Should this info be stored in the JSON instead? It's a property of the catalog, so the information can be specified at write-time instead of at read-time.

i'm definitely in favor of supporting this when it's defined in the JSON and adding it to the spec: https://github.com/intake/intake-esm/blob/main/docs/source/reference/esm-catalog-spec.md

Agreed! I think allowing both here makes sense.

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.

Improve functionality for assets with Attributes containing iterables
4 participants