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

Categorical spatial lag using the Graph #716

Merged
merged 17 commits into from
Jun 7, 2024
Merged

Conversation

sjsrey
Copy link
Member

@sjsrey sjsrey commented May 31, 2024

This has two changes from the creation of categorical spatial lags using W

  1. categorical is now an option in _lag_spatial, to unify treatment of the lag.
  2. This will raise an exception in the case where categorical=True , ties=raise, and ties are encountered.
    "Explicit is better than implicit."

Copy link

codecov bot commented May 31, 2024

Codecov Report

Attention: Patch coverage is 98.61111% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.0%. Comparing base (bcabdbc) to head (82147e8).
Report is 18 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##            main    #716    +/-   ##
======================================
  Coverage   85.0%   85.0%            
======================================
  Files        141     145     +4     
  Lines      15203   15462   +259     
======================================
+ Hits       12924   13149   +225     
- Misses      2279    2313    +34     
Files Coverage Δ
libpysal/graph/base.py 97.0% <ø> (-1.0%) ⬇️
libpysal/graph/tests/test_spatial_lag.py 100.0% <100.0%> (ø)
libpysal/graph/_spatial_lag.py 97.7% <97.4%> (-2.3%) ⬇️

... and 10 files with indirect coverage changes

Copy link
Member

@jGaboardi jGaboardi left a comment

Choose a reason for hiding this comment

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

Three ruff errors (2 linting & 1 formatting) need to to fixed locally and re-pushed.

ruff.....................................................................Failed
- hook id: ruff
- exit code: 1

libpysal/graph/tests/test_spatial_lag.py:1:1: I001 [*] Import block is un-sorted or un-formatted
libpysal/graph/tests/test_spatial_lag.py:62:13: F841 Local variable `yl` is assigned to but never used
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).

ruff-format..............................................................Failed
- hook id: ruff-format
- files were modified by this hook

2 files reformatted, 151 files left unchanged

libpysal/graph/_spatial_lag.py Outdated Show resolved Hide resolved
Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

This needs to be propagated to base.py

def lag(self, y):

libpysal/graph/_spatial_lag.py Outdated Show resolved Hide resolved
libpysal/graph/_spatial_lag.py Show resolved Hide resolved
Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Two minor suggestions in tests.

As mentioned before, this needs to propagate to

def lag(self, y):
"""Spatial lag operator
If weights are row standardized, returns the mean of each
observation's neighbors; if not, returns the weighted sum
of each observation's neighbors.
Parameters
----------
y : array-like
array-like (N,) shape where N is equal to number of observations in self.
Returns
-------
numpy.ndarray
array of numeric values for the spatial lag
"""
return _lag_spatial(self, y)

libpysal/graph/tests/test_spatial_lag.py Outdated Show resolved Hide resolved
libpysal/graph/tests/test_spatial_lag.py Outdated Show resolved Hide resolved
sjsrey and others added 3 commits June 4, 2024 05:16
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
@ljwolf
Copy link
Member

ljwolf commented Jun 4, 2024

What would you think about dispatching to categorical lag if the type is categorical?

I can appreciate that this might make the lag type implicit, but if someone has cast the variable to a categorical already, I think we can infer that they need the appropriate lag function?

@martinfleis
Copy link
Member

@ljwolf that is what we do on l. 87, no?

libpysal/graph/base.py Outdated Show resolved Hide resolved
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Just two minor nits on the docstring. Also, ruff complains at the moment. But once that is solved, happy to merge.

libpysal/graph/base.py Outdated Show resolved Hide resolved
libpysal/graph/base.py Outdated Show resolved Hide resolved
sjsrey and others added 2 commits June 4, 2024 12:49
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
@martinfleis
Copy link
Member

martinfleis commented Jun 7, 2024

pre-commit.ci autofix

@martinfleis martinfleis merged commit 350f81e into pysal:main Jun 7, 2024
9 checks passed
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