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

BUG: fix graph kernel builder when kernel returns zero #569

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

martinfleis
Copy link
Member

When the kernel function returns zeros, the conversion of sparse to arrays drops observations causing a mismatch in the shape of Graph.sparse and original input.

We simply cannot use taper. If there is a good use case for calling eliminate_zeros(), we may revise that but with a smarter implementation to avoid this issue. See the test for reproducible example.

@martinfleis
Copy link
Member Author

covecov messed up :) codecov/codecov-action#1089

including fix here to make CI run

@ljwolf
Copy link
Member

ljwolf commented Sep 15, 2023

While I think we should jettison the eliminate_zeros() call right now, I do think we need to provide for a separate taper() implementation eventually that correctly induces a sparse data structure. Could we set taper=False for now and raise NotImplementedError() when taper==True?

@martinfleis
Copy link
Member Author

Yup, can do that.

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #569 (d3aa1eb) into main (d7b458d) will increase coverage by 0.1%.
The diff coverage is 96.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #569     +/-   ##
=======================================
+ Coverage   81.6%   81.6%   +0.1%     
=======================================
  Files        127     128      +1     
  Lines      14933   14937      +4     
=======================================
+ Hits       12183   12195     +12     
+ Misses      2750    2742      -8     
Files Changed Coverage Δ
libpysal/graph/_kernel.py 71.3% <0.0%> (-0.9%) ⬇️
libpysal/graph/_triangulation.py 98.0% <ø> (ø)
libpysal/graph/_utils.py 92.5% <100.0%> (+0.8%) ⬆️
libpysal/graph/base.py 95.6% <100.0%> (-0.2%) ⬇️
libpysal/graph/tests/test_kernel.py 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@jGaboardi
Copy link
Member

Failures seem to be simple connection errors.

@knaaptime
Copy link
Member

knaaptime commented Sep 17, 2023

what about setting a temporary sentinel value, elim zeros, then replace sentinel with 0?

edit: sorry, scratch that. I see by that time we're already in sparse land, so replacement is hard

@martinfleis
Copy link
Member Author

I'll merge this now. We have the NotImplementedError there so easy to pick it up later.

@martinfleis martinfleis merged commit 0e3daf3 into pysal:main Sep 20, 2023
9 checks passed
@martinfleis martinfleis deleted the fix_kernel branch September 20, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants