pysal / esda

statistics and classes for exploratory spatial data analysis
https://pysal.org/esda
BSD 3-Clause "New" or "Revised" License
206 stars 53 forks source link

Moran.__init__ changes parameter attributes #246

Closed steenrotsman closed 1 year ago

steenrotsman commented 1 year ago

Instantiating a Moran object can change the value of w used for construction, this can produce bugs that are very hard to track down. The reason for this is that w is passed by reference and not by value, so Moran.w is linked to w.

Minimal reproducible example of the behavior:

class Weights:
    def __init__(self):
        self.transform = 'original'

class Moran:
    def __init__(self, w, transformation='changed'):
        w.transform = transformation
        self.w = w

w = Weights()
print(w.transform)
> original
m = Moran(w)
print(w.transform)
> changed
w.transform = 'changed again'
print(m.w.transform)
> changed again

The easiest way to prevent this is using the copy module, so changing

w.transform = transformation
self.w = w

to

self.w = deepcopy(w) # requires from copy import deepcopy
self.w.transform = transformation

Is this change desirable or should I create a different weights object if I want to be sure it's not changed before?

ljwolf commented 1 year ago

Hi! Thanks for the report.

Yes, we’re aware this happens, and can be surprising. As I recall, it happens across any statistic that has a default transform.

I am worried that copying on use would be a breaking change for code that assumes the side effect…

As a user, I would definitely copy the weights each time. Alternatively, setting ‘w.transform = “O”’ after the ‘Moran_Local()’ call should revert any standardization.

We have a proposal that is set to land imminently for the “next generation” ‘W’ object that avoids this kind of side effect/statefulness. My plan is to propagate this into ‘esda’ within six months of its release.

steenrotsman commented 1 year ago

Thanks so much for your response! I agree, code relying on this behavior would break, so best not change it.