lag_spatial

[lanselin]

currently lag_spatial requires a libpysal spatial weights object as the argument w. there is no type or dimension checking and the function only contains w.sparse * y. there is some code in spreg that implements spatial lag computations for both libpysal weights and sparse matrices separately. this seems to be duplicative and asking for trouble.

in the new weights setup, is there a way to make lag_spatial agnostic to the type of weights object passed? i.e., make it work with both libpysal weights objects and sparse matrices without having to make this explicit?

just a thought.

[martinfleis]

In the new graph.Graph approach, lag will be a method of the Graph class, so the input will be only the array of values, no matrix.

If there is a need to directly support sparse arrays, rather than using then under the hood via the Graph class, we could probably keep existing function around and add some shape checks to that. But I am not sure we need that.

[lanselin]

I would definitely suggest keeping the existing function, since removing it would break all kinds of stuff in spreg (and probably in other older code as well). Adding a check in the existing lag_spatial should be straightforward and it could also support graph.Graph without breaking anything.

[knaaptime]

@lanselin do you have any interest in us taking a look at spreg to explore adopting the new Graph internally (eventually) over there? Any internal change would be invisible to users and any external change (e.g. how to manually add a lag variable to a dataframe, etc) would have a good deprecation period and a lot of exposure across the entire ecosystem to help demonstrate the new convention

One of the design principles for the new Graph is that it must be compatible with the way spreg does things, so we're spending a lot of cycles on things like keeping the W/sparse representations aligned so that things like lag operators and higher-order neighbors etc dont break anything in spreg. We liked the idea of method chaining on the weights, and the Graph will still have a .sparse attribute that gives a properly formatted sparse array, any time we need to operate on sparse directly (and a drop-in replacement any time the old lag/W is used directly)

We're trying to do a 1:1 backwards-compatibility check, with spreg in mind, specifically. We've got a 'to_W` to help with testing against the old implementation. We could start a parallel branch of spreg that makes use of the new Graph but runs all the old spreg tests so that it surfaces anything unexpected? If we start digging in now, it might help us make some better design decisions about the Graph

I'm using a lot of spreg at the moment, so i'm happy to do all the heavy lifting and coordinate with you and @pedrovma. But i also dont want to waste your time if you dont want to go that route

[lanselin]

@knaaptime, you should know that @pedrovma and I are currently working on a lot of new functionality for spreg in a private branch of spreg. As we usually proceed, we implement all new code locally and then move it to the main branch for any remaining adjustments in tests etc. that are usually required. Specifically, we have cleaned up and streamlined all the output listings and are adding an SLX option to all current functions (which implements spatial Durbin, GNS etc. as special cases), also in regimes. Also, some new wrapper classes to simplify the use of spatial error methods and in the pipeline are MESS and a general nonlinear SLX distance function. I would suggest waiting with the Graph implementation in spreg until these changes are committed to the official main branch to avoid later conflicts. We're almost there. I have some new testing notebooks that run all permutations of options (several hundreds ...), very handy.

[knaaptime]

Thanks @lanselin that all sounds awesome. I'd spoken to @pedrovma a bit offline about a few these things, but had no idea it was all in the pipeline. Looking forward to it!

I'll hold off until you guys decide to push your stuff up. It would be fun to throw some ideas around at NARSC :)

[ljwolf]

fwiw, I think it's easy to throw a check for the new class in lag_spatial() that dispatches to the new Graph.lag() depending on the type of the first input, as @lanselin suggests... we have not yet made a well-defined transition plan, and I definitely think .lag() is a core concern of the transition plan.