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 NetCDF3 dtype coercion for unsigned integer types #4018

Conversation

blsqr
Copy link
Contributor

@blsqr blsqr commented May 1, 2020

This PR addresses #4014.

  • Closes Add NetCDF3 dtype coercion for unsigned integer types #4014
  • Tests added or adjusted
    • Adjusted a test case where int64 coercion played a role
    • Explicit test for failing coercion (seems to be missing currently -- should this be added?)
  • Passes isort -rc . && black . && mypy . && flake8
  • Fully documented
  • Adjusted whats-new.rst for all changes and api.rst for new API

blsqr added 2 commits May 1, 2020 12:27
This might be a bit too general for what is required at this point,
though ... 🤔
@blsqr
Copy link
Contributor Author

blsqr commented May 1, 2020

Currently, the error that is raised by coerce_nc3_dtype is not covered by tests.

A test could look something like this:

    def test_dtype_coercion_error(self):
        for dtype in _nc3_dtype_coercions:
            if dtype == "bool":
                continue

            # Create some data that cannot be coerced (cause it's a downcast)
            dtype = np.dtype(dtype)
            maxval = np.iinfo(dtype).max
            x = np.array([0, 1, 2, maxval], dtype=dtype)
            test_data = Dataset({"x": ("t", x, {})})

            # Make sure a roundtrip fails
            with pytest.raises(ValueError, match="could not safely cast"):
                self.roundtrip(test_data)

... but I'm not sure ...

  • whether this would be desirable to test
  • whether this PR should take care of this or not
  • where in test_backend.py I would have to put the test case such that it's invoked only for netcdf3.
    • I already found the NetCDF3Only class and requires_scipy decorator, but don't know which test class is responsible here ...

@blsqr blsqr marked this pull request as ready for review May 1, 2020 13:31
@blsqr
Copy link
Contributor Author

blsqr commented May 1, 2020

@dcherian I think this would be ready for review now. I'd appreciate some guidance on the implementation of the tests; see above.

@dcherian
Copy link
Contributor

dcherian commented May 1, 2020

If you are up for it, we'd love to add testing for that error in this PR.

I think NetCDF3Only is the right place to put it. It gets called by a number of test classes that inherit from NetCDF3Only. Add it there and see if things pass?

@blsqr
Copy link
Contributor Author

blsqr commented May 2, 2020

Alright, I added the test. From my side, this would be ready. :)

Let me know if further changes to the tests or What's New are necessary.

@dcherian dcherian mentioned this pull request May 5, 2020
23 tasks
@dcherian
Copy link
Contributor

dcherian commented May 8, 2020

LGTM. Thanks @blsqr

@dcherian
Copy link
Contributor

Yay. Thanks @blsqr. I see this is your first PR. welcome!

@dcherian dcherian merged commit 5c04ebf into pydata:master May 20, 2020
@blsqr
Copy link
Contributor Author

blsqr commented May 20, 2020

Thanks, @dcherian! I'm happy to contribute 😊

@blsqr blsqr deleted the 4014-add-netcdf-dtype-coercion-for-unsigned-integer-types branch July 13, 2020 15:37
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.

Add NetCDF3 dtype coercion for unsigned integer types
2 participants