8000 BUG: fix graph kernel builder when kernel returns zero by martinfleis · Pull Request #569 · pysal/libpysal · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 20, 2023
Merged

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

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 •
edited
Loading

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 jGaboardi requested a review from ljwolf September 15, 2023 13:47
@jGaboardi
Copy link
Member

Failures seem to be simple connection errors.

@knaaptime
Copy link
Member
knaaptime commented Sep 17, 2023 •
edited
Loading

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
@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
Reviewers

@jGaboardi jGaboardi jGaboardi approved these changes

@knaaptime knaaptime Awaiting requested review from knaaptime

@ljwolf ljwolf Awaiting requested review from ljwolf

Assignees

@martinfleis martinfleis

Labels
graph
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martinfleis @ljwolf @jGaboardi @knaaptime
0