add from_adjacency and handle sorting #555
Conversation
|
failures are expected because the 'known order' is different. So the failures should be specific to the placement of neighbor values, but i left the tests unchanged for demonstration I'm not sure why 3.8 minimal is failing sparse tests |
| adj = ( | ||
| self.adjacency.reset_index() | ||
| .set_index(["focal", "neighbor"])["weight"] | ||
| .astype("Sparse[float]") | ||
| .reindex(ids, level=0) | ||
| .reindex(ids, level=1) | ||
| ) | ||
| # pivot to COO sparse matrix and cast to array | ||
| return sparse.coo_array(adj.sparse.to_coo(sort_labels=True)[0]) |
There was a problem hiding this comment.
If we use the _id2i here anyway, what is the benefit of reindexing, casting to pandas sparse, to scipy sparse matrix and then to a sparse array over the direct constructor we had here?
There was a problem hiding this comment.
Also I’m not sure we’re technically need id2i anymore because we could do self.adjacency.focal.unique() to get the ids which should always be in the correct order, but I think it’s still useful to keep the order stashed as a private attribute
There was a problem hiding this comment.
i also think that whenever possible we should rely on indexing rather than mapping by hand, as indices are really the benefit that the adjlist-backed implementation provides. Frankly, the multiindexed series is what we should really use under the hood at all times. That's what i sketched in the original implementation and im not sure why we've diverged. If we had the adjacency stored as multiindex sparse on init, all we'd need to do here is .to_coo
There was a problem hiding this comment.
We originally started with a Series with a MultiIndex but there was something funny about sorting messing up sparse in some cases. But we may have prematurely dropped the idea and resorted to DataFrame. I also think that a Series would be optimal data structure. Also from performance perspective.
Given how much the code has changed since then and that we do this sorting now, we may want to revise that and go back to Series. It should not be a complex change. We need to adapt from_arrays and init and maybe throw in a couple of reset_index here and there.
There was a problem hiding this comment.
All for revisiting that. We jettisoned it because of sorting issues, but it's the correct data structure and we started out intending to keep @knaaptime's core approach
There was a problem hiding this comment.
I think that the requirement to keep a MultiIndex lexicographically sorted to do anything performant was the main issue. pandas does not like indices that are not sorted (see this section in their docs). In some cases, they even raise for unsorted indices, though that may not be our case.
While we do have indices sorted, they are sorted according to the order of appearance in the original dataframe, which is not sorted in pandas terms. So while I would like to store adjacency as a mutli-indexed series, I am not sure it is possible and if it is possible, if it is actually a good idea.
There was a problem hiding this comment.
That said, I am also happy to give it a try so we're not just guessing here.
| ids = numpy.unique(numpy.hstack((heads, tails))) | ||
| ids = np.unique(np.hstack((heads, tails))) |
There was a problem hiding this comment.
Given the requirement for the presence of every observation in focal, at least as a self-loop, the hstack is not needed anymore.
for discussion