CartoDB / crankshaft

CARTO Spatial Analysis extension for PostgreSQL
BSD 3-Clause "New" or "Revised" License
54 stars 20 forks source link

Upgrade pysal to version 1.14.3 #198

Closed rafatower closed 6 years ago

rafatower commented 6 years ago

This solves a problem with the Markov analysis. Otherwise, with some inputs it gives the following error:

Analysis A2 failed:
ValueError: operands could not be broadcast together with shapes (5) (3)

The stack trace is the following:

ERROR:  ValueError: operands could not be broadcast together with shapes (5) (2)
CONTEXT:  Traceback (most recent call last):
  PL/Python function "cdb_spatialmarkovtrend", line 7, in <module>
    return markov.spatial_trend(subquery, time_cols, num_classes, w_type, num_ngbrs, permutations, geom_col, id_col)
  PL/Python function "cdb_spatialmarkovtrend", line 76, in spatial_trend
  PL/Python function "cdb_spatialmarkovtrend", line 416, in __init__
  PL/Python function "cdb_spatialmarkovtrend", line 498, in _mn_test
  PL/Python function "cdb_spatialmarkovtrend", line 526, in _ssmnp_test
PL/Python function "cdb_spatialmarkovtrend"

and here is the line where it actually fails: https://github.com/pysal/pysal/blob/v1.11.2/pysal/spatial_dynamics/markov.py#L526

In v1.14.3, the shtest is opt-in. So this gives us a chance to curate data if we wanted to apply that test, optionally, instead of crashing:

https://github.com/pysal/pysal/blob/v1.14.3/pysal/spatial_dynamics/markov.py#L490

rafatower commented 6 years ago

Hey @andy-esch: I could not come up with a python unit test for this case. I think that adding a sql test for it makes little sense, and since it does not break any test, if you don't have any problem with it I'd go ahead with this change and release along with your other changes in develop branch.

andy-esch commented 6 years ago

That sounds good to me. I successfully installed this branch locally and all the tests (python and sql) passed.