Handle set_index properly in to_adjlist

[sjsrey]

xref #507

[codecov]

Codecov Report

Merging #511 (7cca4e5) into master (49c69b6) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff            @@
##           master    #511     +/-   ##
========================================
+ Coverage    78.7%   78.8%   +0.1%     
========================================
  Files         122     122             
  Lines       13182   13225     +43     
========================================
+ Hits        10371   10417     +46     
+ Misses       2811    2808      -3     

Impacted Files	Coverage Δ
libpysal/weights/tests/test_adjlist.py	98.4% <100.0%> (+0.7%)	⬆️
libpysal/weights/weights.py	83.2% <100.0%> (+0.5%)	⬆️
libpysal/weights/contiguity.py	77.8% <0.0%> (+0.5%)	⬆️

[martinfleis]

We don't need to do this if there are no custom ids set. I would add

self.ids = ids

to the W.__init__ and then have this done conditionally after

if self.ids:
    ...

You can potentially also use the ids directly here but that does not matter much

        if self.ids:
            ids = np.asarray(self.ids)
            focal_ix = ids.take(focal_ix)
            neighbor_ix = ids.take(neighbor_ix)

Using keys from neighbours is probably safer option anyway.

[knaaptime]

martin's review echos my last two comments in #507, so do we also want to expose the ids attribute and only conditionally updated the adjlist when they dont match? (as a part of this PR?)

[martinfleis]

do we also want to expose the ids attribute

self.ids shall be identical as neigbors.keys so not sure if it is worth exposing as a public attribute.

only conditionally updated the adjlist when they don't match

I'd surely do that (as above)

[knaaptime]

self.ids shall be identical as neigbors.keys so not sure if it is worth exposing as a public attribute.

since we're getting rid of id_order (and id2i), we need to at least store the ids, otherwise conversion to wsp will lose the indices

since we need to store them anyway, we might as well make it a public attribute. Might be a little more transparent for users instead of hitting w.neighbors.keys

[martinfleis]

Okay, nothing against that.

[knaaptime]

or--when converting to WSP, we could stash w.neighbors.keys as a private attribute, then use that when going back WSP --> W (in which case we just leave everything as-is)

[martinfleis]

It seems safer to have an attribute always available than that.

[knaaptime]

yeah agree. And looking a bit more closely, we use id_order a lot in the sparse stuff, so a straight swap-out for ids seems like the best choice there also

[knaaptime]

lets go ahead and merge this. I've got another branch that includes the enhancements we discussed in this thread, but touches a bunch of other stuff to get rid of id_order, so lets get this in and i can rebase that for review

[martinfleis]

@knaaptime can you ensure in the follow-up that the remapping here is done only if needed, ie. if ids are set?

[knaaptime]

yeah but it'll be in a separate pr from this branch as that one does away with id_order