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

use int32 as default int representation in windows #1179

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cosmicBboy
Copy link
Collaborator

fixes #726

Signed-off-by: Niels Bantilan <[email protected]>
@codecov
Copy link

codecov bot commented May 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.20 ⚠️

Comparison is base (a057df0) 97.23% compared to head (7a80419) 97.03%.

❗ Current head 7a80419 differs from pull request most recent head 93eadaa. Consider uploading reports for the commit 93eadaa to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1179      +/-   ##
==========================================
- Coverage   97.23%   97.03%   -0.20%     
==========================================
  Files          65       65              
  Lines        5067     5067              
==========================================
- Hits         4927     4917      -10     
- Misses        140      150      +10     
Impacted Files Coverage Δ
pandera/engines/pandas_engine.py 95.44% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Niels Bantilan <[email protected]>
@cosmicBboy cosmicBboy changed the title test default windows int use int32 as default int representation in windows May 9, 2023
@eapolinario
Copy link

/review

@flyte-bot
Copy link

flyte-bot commented Dec 27, 2024

Code Review Agent Run #722777

Actionable Suggestions - 1
  • tests/core/test_dtypes.py - 1
    • Platform check may not reflect architecture · Line 511-514
Additional Suggestions - 1
  • tests/core/test_checks.py - 1
    • Consider simplifying line continuation usage · Line 28-30
Review Details
  • Files reviewed - 3 · Commit Range: 7a80419..93eadaa
    • pandera/engines/pandas_engine.py
    • tests/core/test_checks.py
    • tests/core/test_dtypes.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by Bito Logo

@flyte-bot
Copy link

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Bug Fix - Windows Integer Type Compatibility Fix

pandas_engine.py - Modified default integer dtype to use OS-specific types (int32 for Windows, int64 for others)

test_checks.py - Updated test cases to explicitly specify integer dtype in Series

test_dtypes.py - Added platform-specific tests for default integer dtype

Comment on lines +511 to +514
if platform.system() == "Windows":
assert default_int_dtype == np.dtype("int32")
else:
assert default_int_dtype == np.dtype("int64")

Choose a reason for hiding this comment

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

Platform check may not reflect architecture

Consider using platform.machine() instead of platform.system() to check for architecture since integer size is typically architecture dependent rather than OS dependent. On 32-bit systems, default int is 32-bit while on 64-bit systems it's 64-bit.

Code suggestion
Check the AI-generated fix before applying
Suggested change
if platform.system() == "Windows":
assert default_int_dtype == np.dtype("int32")
else:
assert default_int_dtype == np.dtype("int64")
if platform.machine() in ["i386", "x86"]:
assert default_int_dtype == np.dtype("int32")
else:
assert default_int_dtype == np.dtype("int64")

Code Review Run #722777


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

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.

Default int type should be mapped to Int32 in Windows
3 participants