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

Test formatting platform #8719

Merged
merged 7 commits into from
Feb 9, 2024

Conversation

etienneschalk
Copy link
Contributor

@etienneschalk etienneschalk commented Feb 7, 2024

Follow up #8702 / #8702 (comment)

The goal is to remove the not elegant OS-dependant checks introduced during the testing of #8702

A simple way to do so is to use unsigned integer as dtypes for tests involving data array representations on multiple OSes. Indeed, this solves the issue of the default dtypes being not printed in the repr, with default dtyps varying according to the OS. The tests show that the concerned dtypes are int32 (for the Windows CI) and int64 (for Ubuntu and macOS CIs). Using uint64 should fix both the varying size and the varying numpy array repr.

- [ ] Closes #xxxx

  • Tests added
    - [ ] User visible changes (including notable bug fixes) are documented in whats-new.rst
    - [ ] New functions/methods are listed in api.rst

@etienneschalk
Copy link
Contributor Author

@max-sixty (answering to your comment on the previous merged PR #8702)

I created an issue on numpy, to propose an option to force print the dtype, that could be useful in a testing context numpy/numpy#25787

I thought about regexps too but... this it adds more complex and more error-prone logic to the tests, results are also less human-readable, and

In this draft PR, I wrote 3 tests:

  • One with all dtypes leading to platform-independant reprs
  • One for Windows
  • One for the other OSes

We can see that in our CI, the most problematic dtypes are integers int32 (default on Windows) and int64 (default on non-Windows).

Unsigned integers do not pose any problem as they are always printed out in a repr. Also, as they weigh as much as their signed counterpart, they are good candidates to:

  • Write OS-independant code
  • Not having to update the sizes in the reprs
  • Limitation: the actual data should not be useful for the test, eg dummy arrays like [1, 2, 3] could be easily converted to unsigned

@max-sixty
Copy link
Collaborator

Yes very impressive tests!

I like the idea of using unsigned ints, that's a great idea. If you're up for doing that, I would vote for that option.

@etienneschalk etienneschalk marked this pull request as ready for review February 8, 2024 19:33
Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

I think we might be over testing here, such that if we make another change then it'll be a lot of work to update! But that's hardly a primary concern, and very much appreciate the work to refactor the initial OS-dependent tests @etienneschalk . Thank you!

@max-sixty max-sixty enabled auto-merge (squash) February 9, 2024 02:26
@max-sixty max-sixty merged commit 0ad7fa7 into pydata:main Feb 9, 2024
28 checks passed
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.

2 participants