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 fill_value for concat and auto_combine #2964

Merged
merged 5 commits into from
May 27, 2019

Conversation

zdgriffith
Copy link
Contributor

This PR adds the optional fill_value parameter to concat() and auto_combine() in xarray.core.combine, plus the associated underlying functions in the module. This builds on the previous PR #2920 in addressing issue #2876, and gives users the option to avoid type-changing to float for arrays with missing values when using these functions as discussed in issue #2870.

@zdgriffith zdgriffith changed the title Add fill_value for concat and auto_combine methods Add fill_value for concat and auto_combine May 15, 2019
@dcherian
Copy link
Contributor

@TomNicholas looks like you'll need to add fill_value to combine_auto and combine_manual?

@TomNicholas
Copy link
Member

Yeah I saw this - that's fine. Probably best to do them as separate commits though no? Just depends which one gets merged first. If we merge mine first then I'm happy to review/adapt this PR to be compatible.

@dcherian
Copy link
Contributor

👍. Mind giving this PR a review? You know this bit of the code well...

@dcherian dcherian mentioned this pull request May 21, 2019
15 tasks
@shoyer
Copy link
Member

shoyer commented May 21, 2019

This looks pretty safe to me. It's basically just threading down the fill_value argument.

@TomNicholas
Copy link
Member

TomNicholas commented May 21, 2019

Yeah this looks fine to me - it basically bypasses all the complicated combining logic anyway.

Just to be totally explicit then changing _auto_combine() obviously means that open_mfdataset() will have an implicit default kwarg fill_value='dtypes.NA', but if that's always the desired behaviour then that's fine.

@shoyer
Copy link
Member

shoyer commented May 21, 2019

Just to be totally explicit then changing _auto_combine() obviously means that open_mfdataset() will have an implicit default kwarg fill_value='dtypes.NA', but if that's always the desired behaviour then that's fine.

This is the current default behavior for open_mfdataset. We could potentially had a fill_value option there, too.

@shoyer shoyer merged commit 6dc8b60 into pydata:master May 27, 2019
@shoyer
Copy link
Member

shoyer commented May 27, 2019

Thanks @zdgriffith !

dcherian added a commit to dcherian/xarray that referenced this pull request May 29, 2019
* upstream/master:
  cfgrib is now part of conda-forge (pydata#2992)
  Add fill_value for concat and auto_combine (pydata#2964)
  Remove deprecated pytest.config usages (pydata#2988)
  Add transpose_coords option to DataArray.transpose (pydata#2556)
  Fix rolling.constuct() example (pydata#2967)
  Implement load_dataset() and load_dataarray() (pydata#2917)
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.

xr.concat changes dtype
4 participants