Skip to content

Clean up the data in test_type_promotion.py #21

Closed

Description

@asmeurer

Everything that's not a test in test_type_promotion.py is a mess right now. There are tons of data dictionaries and helper functions. Quite a few of them are needed throughout the test suite, so they should be moved to a more central location. Furthermore, the data has a lot of redundancies, and even so, to use it, you often have to do tons of dict lookups to get at what you want. I think the biggest problem here is that I used the string values of the dtypes as the main key in the dictionaries, meaning you always have to translate those to actual dtype objects to use them. We should just use the actual dtype objects instead. The only concern here is that the dtype objects might not be hashable. But I don't think this is an actual problem in practice. If it somehow does become a problem for some library, we can worry about it then, and whether it should be worked around in the suite or if we should just require hashability in the spec (we already require equality). Finally, the parameterization logic for the tests that are in test_type_promotion is a bunch of hard to read list comprehensions. They may become easier to read once we refactor the data. If not, we should clean them up as well. So to summarize

  • Move the reusable data and helper functions to a central location (I'm thinking a new file, like dtype_helpers.py). The input and output type mappings should also be moved, as they are also useful outside of just these tests.
  • Change the data to be keyed on the dtype objects themselves. I don't actually care if we keep the "i4" type names anywhere. Those are used in the spec, but they aren't actually that clear. I would rather we just use the dtype name itself, like "int32", everywhere, even in the places where the string names are currently used (I think it's only in the type promotion parameterization keys).
  • Other refactors like renaming variables to better names are also welcome.
  • Clean up the parameterization logic in test_type_promotion.py

Activity

  1. honno commented on Oct 5, 2021

    @honno
    Member

    Working on this, thanks for the checklist. Will let you know if I have any questions.

  2. honno commented on Oct 11, 2021

    @honno
    Member

    RE hashability, worst case there's probably a hack we can do in _array_module to wrap dtypes in hashable items (i.e. use dtype string names as hashes). This might be useful anyway so we can get consistent str(dtype) output for things like error messages and pytest parameters.

  3. asmeurer commented on Oct 13, 2021

    @asmeurer
    MemberAuthor

    That would mean we'd have to unpack the dtype object everywhere it is used, though. Let's not worry about this unless/until it becomes an issue for some actual library. I doubt any of the existing libraries have unhashable dtypes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions