Matching

[ljwolf]

Hello! Please make sure to check all these boxes before submitting a Pull Request
(PR). Once you have checked the boxes, feel free to remove all text except the
justification in point 5.

1. You have run tests on this submission locally using pytest on your changes. Continuous integration will be run on all PRs with GitHub Actions, but it is good practice to test changes locally prior to a making a PR.
2. This pull request is directed to the pysal/main branch.
3. This pull introduces new functionality covered by
   docstrings and
   unittests?
4. You have assigned a
   reviewer and added relevant labels
5. The justification for this PR is contained in Interest in Optimal Spatial Matching?  #665

todo:

• add pulp to our testing matrix.
• write tests using the toy example in the gist
• write an example notebook

[codecov]

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (e24adfb) 85.0% compared to head (a24c979) 85.0%.
Report is 18 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #666     +/-   ##
=======================================
- Coverage   85.0%   85.0%   -0.0%     
=======================================
  Files        139     141      +2     
  Lines      14889   15023    +134     
=======================================
+ Hits       12663   12771    +108     
- Misses      2226    2252     +26     

Files	Coverage Δ
libpysal/graph/base.py	97.8% <100.0%> (+0.1%)	⬆️
libpysal/graph/tests/test_builders.py	100.0% <100.0%> (ø)
libpysal/graph/tests/test_matching.py	97.9% <97.9%> (ø)
libpysal/graph/_matching.py	72.0% <72.0%> (ø)

... and 3 files with indirect coverage changes

[ljwolf]

Tests are now added, but pulp installation fails on windows through conda-forge. Is there a better way to do this?

[jGaboardi]

Tests are now added, but pulp installation fails on windows through conda-forge. Is there a better way to do this?

In spopt we install pulp via pip --> https://github.com/pysal/spopt/blob/4760bd9e03f9b31c003bfa7f5d785a2a81b174bd/ci/312-latest.yaml#L32

[jGaboardi]

Looks like numpy is never being imported --> https://github.com/pysal/libpysal/actions/runs/7159612453/job/19493044782?pr=666#step:7:2098

[jGaboardi]

Is this passing locally for you?

[knaaptime]

i think it just needs to be wrapped in a tuple

[ljwolf]

Yes, but fixing that, I still have fails locally. Will fix.

[ljwolf]

One question I had for maintainers:

I built the spatial matching framework to be able to do cross-pattern queries. So, something like:

For all points in pattern X, find at least k matched points in pattern Y such that the total distance of X->Y matches are minimised.

I think this would be good to expose in cg, since it's a general purpose spatial matching function for computational geometry applications. I have something even more general that will probably make it into a separate package for spatial causal inference in python, but the spatial implementation can live here.

So, would it be OK to grab _spatial_matching() from libpysal.graph._matching and expose it directly in libpysal.cg? I could alternatively define a dispatching function that requires the second pattern (y argument) to be provided.

[jGaboardi]

I'm wondering if there is way to simply choose the default installed solver without needing to specific (or is this what's happening here with pulp.COIN()?). Implementing that logic would go a long way over in spopt >> pysal/spopt#348

cc @gegen07

[ljwolf]

I think we need to do this, since pulp.PULP_CBC_CMD() I think is different from pulp.COIN_CMD(), and some students could run pulp.GUROBI() but not pulp.GUROBI_CMD().

I think we should really leave it to pulp to deal with this, rather than specifying COIN.

[jGaboardi]

Yeah, seems like it would make a nice improvement in spopt to have the ability to pass in no solver, and then the default one installed is used within the solve() methods.

[ljwolf]

I think just delegating to the model's solve method will choose a default available solver, if one is configured. We shouldn't intercept this to set a default (imho).

[ljwolf]

The code is ready to review. I'm not sure why the ruff check is failing... I've run the ruff auto formatter myself locally on the submission (2c04acf, 215022d, 273e6ea)

[jGaboardi]

The code is ready to review. I'm not sure why the ruff check is failing... I've run the ruff auto formatter myself locally on the submission (2c04acf, 215022d, 273e6ea)

Do you have the most recent ruff installed locally? I think there was a fresh version released last night. Could be playing a part.

[ljwolf]

Yes, not quite sure what's up here:

0.1.8 is the most recent version of ruff, and it fixes nothing both in the CI and locally?

[martinfleis]

It is ruff-format that is failing, not ruff linter.

[ljwolf]

OK! Wasn't clear on the differences between ruff check and format. Sorry!

I think this is complete.

[jGaboardi]

Looking very cool.

[martinfleis]

The large diff in base is just moving stuff around to get alphabetic order of builders?

Can you also add a test for Graph.build_spatial_matches ?

[jGaboardi]

The large diff in base is just moving stuff around to get alphabetic order of builders?

Yes.

[ljwolf]

I believe I've addressed all comments!

[ljwolf]

Hi! could one of the approvers please merge this?

[ljwolf]

thank you!