Skip to content

BUG: cast integers to floats for numba compatibility in Getis-Ord - #442

Closed
samay2504 wants to merge 4 commits into
pysal:mainfrom
samay2504:fix/issue-275-g-local-int-dtype
Closed

BUG: cast integers to floats for numba compatibility in Getis-Ord#442
samay2504 wants to merge 4 commits into
pysal:mainfrom
samay2504:fix/issue-275-g-local-int-dtype

Conversation

@samay2504

@samay2504 samay2504 commented Mar 31, 2026

Copy link
Copy Markdown
Contributor

This PR fixes issue #275 by making G_Local robust to integer and mixed numeric input dtypes that previously triggered a Numba TypingError during conditional randomization: we now normalize the input array to a float dtype in the local-statistic path so matrix operations used in permutation inference are dtype-consistent, while preserving existing public behavior and outputs for standard float inputs; to prevent regression, verifies G_Local runs successfully after the fix in esda/getisord.py.

Copilot AI review requested due to automatic review settings March 31, 2026 18:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses issue #275 by ensuring esda.getisord.G_Local can accept integer (and mixed numeric) inputs without triggering Numba dtype TypingErrors during conditional randomization.

Changes:

  • Cast y to a float dtype at G_Local initialization to keep permutation-inference matrix operations dtype-consistent.
  • Refactor the transform validation assertion formatting in _infer_star_and_structure_w.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread esda/getisord.py
Comment thread esda/getisord.py Outdated
Comment on lines +598 to +601
assert transform.lower() in (
"r",
"b",
), f'Transforms must be binary "b" or row-standardized "r".Recieved: {transform}'

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

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

Avoid using assert for validating user inputs like transform; assertions can be stripped with Python optimizations (-O), which would bypass this check. Prefer raising a ValueError (or TypeError if appropriate) with the same message.

Suggested change
assert transform.lower() in (
"r",
"b",
), f'Transforms must be binary "b" or row-standardized "r".Recieved: {transform}'
if transform.lower() not in (
"r",
"b",
):
raise ValueError(
f'Transforms must be binary "b" or row-standardized "r".Recieved: {transform}'
)

Copilot uses AI. Check for mistakes.
Comment thread esda/getisord.py Outdated
@codecov

codecov Bot commented Mar 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.6%. Comparing base (74ca549) to head (c98efde).
⚠️ Report is 53 commits behind head on main.

Files with missing lines Patch % Lines
esda/getisord.py 0.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #442     +/-   ##
=======================================
+ Coverage   82.2%   83.6%   +1.4%     
=======================================
  Files         27      27             
  Lines       3969    3850    -119     
=======================================
- Hits        3264    3220     -44     
+ Misses       705     630     -75     
Files with missing lines Coverage Δ
esda/getisord.py 68.4% <0.0%> (-2.2%) ⬇️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Comment thread esda/getisord.py
Comment thread esda/getisord.py Outdated
other_weights = weights_i[1:]
zi, zrand = _prepare_univariate(i, z, permuted_ids, other_weights)
return (zrand @ other_weights) / (scaling - zi)
weighted_sum = _rowwise_weighted_sum(zrand, other_weights)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you elaborate why we suddenly need to define this ourselves?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for flagging this @martinfleis .I added that explicit row wise accumulation because Numba fails on integer typed matrix vector @ in this path (BLAS-backed dot does not support those integer signatures), while the loop keeps the computation in nopython mode and preserves integer input support without casting the user data to float. If you prefer, I can refactor this into a shared utility or switch to any equivalent Numba-safe pattern you’d rather standardize on.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did not realise this... In that case, maybe the solution that was in 189d5b4 might be better. Less code to maintain.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @martinfleis , I agree the change in commit 189d5b4 is simpler; I’ll revert to that approach so the numba path supports integer inputs without casting, run the relevant tests locally, and push a minimal update to this PR.

@martinfleis martinfleis changed the title bug #275 resolve BUG: cast integers to floats for numba compatibility in Getis-Ord Jun 1, 2026
@martinfleis

Copy link
Copy Markdown
Member

This bug no longer exists. It has been resolved by #281 and can no longer be reproduced.

@martinfleis martinfleis closed this Jun 1, 2026
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.

3 participants