issue_comments

Table actions

9 rows where issue = 292653302 sorted by updated_at descending

✎ View and edit SQL

This data as json, CSV (advanced)

Suggested facets: reactions, created_at (date), updated_at (date)

user 3

author_association 2

issue 1

  • Add '_FillValue' to set of valid_encodings for netCDF4 backend · 9 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
365360917 https://github.com/pydata/xarray/pull/1869#issuecomment-365360917 https://api.github.com/repos/pydata/xarray/issues/1869 MDEyOklzc3VlQ29tbWVudDM2NTM2MDkxNw== shoyer 1217238 2018-02-13T18:34:44Z 2018-02-13T18:34:44Z MEMBER

Yes, thanks @czr137 !

 
{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add '_FillValue' to set of valid_encodings for netCDF4 backend 292653302
365352947 https://github.com/pydata/xarray/pull/1869#issuecomment-365352947 https://api.github.com/repos/pydata/xarray/issues/1869 MDEyOklzc3VlQ29tbWVudDM2NTM1Mjk0Nw== jhamman 2443309 2018-02-13T18:07:44Z 2018-02-13T18:07:44Z MEMBER

@shoyer - this looks good to me. At some point, we should formalize how backends validate their encodings but I don't think that needs to happen here.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add '_FillValue' to set of valid_encodings for netCDF4 backend 292653302
364993025 https://github.com/pydata/xarray/pull/1869#issuecomment-364993025 https://api.github.com/repos/pydata/xarray/issues/1869 MDEyOklzc3VlQ29tbWVudDM2NDk5MzAyNQ== czr137 6153603 2018-02-12T17:12:23Z 2018-02-12T17:12:23Z CONTRIBUTOR

@shoyer, I think everything is in order to apply this bug fix.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add '_FillValue' to set of valid_encodings for netCDF4 backend 292653302
362097521 https://github.com/pydata/xarray/pull/1869#issuecomment-362097521 https://api.github.com/repos/pydata/xarray/issues/1869 MDEyOklzc3VlQ29tbWVudDM2MjA5NzUyMQ== shoyer 1217238 2018-01-31T22:49:00Z 2018-01-31T22:49:00Z MEMBER

We don't need any additional documentation, but we should note this bug fix in "What's new"

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add '_FillValue' to set of valid_encodings for netCDF4 backend 292653302
362092065 https://github.com/pydata/xarray/pull/1869#issuecomment-362092065 https://api.github.com/repos/pydata/xarray/issues/1869 MDEyOklzc3VlQ29tbWVudDM2MjA5MjA2NQ== czr137 6153603 2018-01-31T22:26:29Z 2018-01-31T22:26:29Z CONTRIBUTOR

@shoyer, do you have an opinion on whether or not to include the scipy fix or leave it out of this PR? Also what is your opinion on the documentation? I feel nothing is needed as with this PR the code will operate as the documentation states.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add '_FillValue' to set of valid_encodings for netCDF4 backend 292653302
361766665 https://github.com/pydata/xarray/pull/1869#issuecomment-361766665 https://api.github.com/repos/pydata/xarray/issues/1869 MDEyOklzc3VlQ29tbWVudDM2MTc2NjY2NQ== czr137 6153603 2018-01-30T23:11:57Z 2018-01-30T23:48:44Z CONTRIBUTOR

Now the tests pass (see comment beside the checkbox)

Honestly, I'm not happy with where I put the fix in order to get the tests to pass when using the scipy backend by adding an additional test, but I didn't know where else to put the test. The test to see if the variable would be checked in prepare_variables came before the encoding could be managed in encode_cf_variable in conventions.py

The alternative would be to remove the changes to scipy_.py and just have the test run only for the other engine backends and leave fixing the scipy backend for someone more familiar with it for another issue.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add '_FillValue' to set of valid_encodings for netCDF4 backend 292653302
361736359 https://github.com/pydata/xarray/pull/1869#issuecomment-361736359 https://api.github.com/repos/pydata/xarray/issues/1869 MDEyOklzc3VlQ29tbWVudDM2MTczNjM1OQ== czr137 6153603 2018-01-30T21:14:52Z 2018-01-30T21:14:52Z CONTRIBUTOR

Wait to merge... I'm still getting failed tests using the scipy backend

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add '_FillValue' to set of valid_encodings for netCDF4 backend 292653302
361734459 https://github.com/pydata/xarray/pull/1869#issuecomment-361734459 https://api.github.com/repos/pydata/xarray/issues/1869 MDEyOklzc3VlQ29tbWVudDM2MTczNDQ1OQ== czr137 6153603 2018-01-30T21:07:36Z 2018-01-30T21:07:36Z CONTRIBUTOR

Unit tests added in 9210dfa45428842c69b8d1835013159b1230c572. The first and the third added tests use the encoding kwarg and fail without b8fd86d5614d28254406d6f9b6d4e37383efc1be applied and pass with b8fd86d5614d28254406d6f9b6d4e37383efc1be applied, so they should catch a regression.

The second added test passes both before and after b8fd86d5614d28254406d6f9b6d4e37383efc1be and was added for completeness as it pairs with the original test_explicitly_omit_fill_value, but for a coordinate.

I still think nothing needs to change in the documentation as this now follows the intent of the design.

I will leave it to you @shoyer or your delegate to address whether anything needs to be stated in the changelog / what's new.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add '_FillValue' to set of valid_encodings for netCDF4 backend 292653302
361488957 https://github.com/pydata/xarray/pull/1869#issuecomment-361488957 https://api.github.com/repos/pydata/xarray/issues/1869 MDEyOklzc3VlQ29tbWVudDM2MTQ4ODk1Nw== shoyer 1217238 2018-01-30T06:20:08Z 2018-01-30T06:20:08Z MEMBER

Oops! Yes, this is was definitely a bug.

Can you add a unit test for this, so we don't have a regression?

Take a look at the test test_explicitly_omit_fill_value added in https://github.com/pydata/xarray/pull/1659 and test_encoding_same_dtype below it for examples.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add '_FillValue' to set of valid_encodings for netCDF4 backend 292653302

Advanced export

JSON shape: default, array, newline-delimited, object

CSV options: 

CREATE TABLE [issue_comments] (
   [html_url] TEXT,
   [issue_url] TEXT,
   [id] INTEGER PRIMARY KEY,
   [node_id] TEXT,
   [user] INTEGER REFERENCES [users]([id]),
   [created_at] TEXT,
   [updated_at] TEXT,
   [author_association] TEXT,
   [body] TEXT,
   [reactions] TEXT,
   [performed_via_github_app] TEXT,
   [issue] INTEGER REFERENCES [issues]([id])
);
CREATE INDEX [idx_issue_comments_issue]
    ON [issue_comments] ([issue]);
CREATE INDEX [idx_issue_comments_user]
    ON [issue_comments] ([user]);