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

Fix longlong timestamps #276

Merged
merged 3 commits into from Oct 5, 2023
Merged

Fix longlong timestamps #276

merged 3 commits into from Oct 5, 2023

Conversation

DonBraulio
Copy link
Collaborator

TL;DR: This PR fixes some issues in normalize_timestamps(), simplifying the code, avoiding issues with numpy dtypes, and improving the error handling by avoiding unnecessary checks.

For context, this fixes an issue that showed up in a notebook by @achoum, caused by numpy representing numbers using np.longlong internally instead of the usual np.int64, after loading a CSV from pandas. This can be reproduced by doing:

>>> np.array([1, 2], dtype=np.longlong).dtype
dtype('int64')
>>> np.array([1, 2], dtype=np.longlong).dtype.type
<class 'numpy.longlong'>

The bug is solved by using np.issubdtype(np.dtype, np.integer), instead of accessing my_array.dtype.type which refers to the underlying scalar type, and there are a bunch of those (see Built in scalar types).

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Coverage report

Main: 90.40% | PR: 90.47% | Diff: 0.08 ✅

Copy link
Collaborator

@achoum achoum left a comment

Choose a reason for hiding this comment

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

Awesome.
A few nits

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Coverage report

Main: 90.40% | PR: 90.50% | Diff: 0.10 ✅

@DonBraulio DonBraulio merged commit 435871f into main Oct 5, 2023
20 checks passed
@DonBraulio DonBraulio deleted the fix-int-timestamp branch October 5, 2023 19:34
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.

None yet

2 participants