Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

insert zeros on symmetric matrix diagonal #660

Closed
wants to merge 1 commit into from

Conversation

dfolch
Copy link
Member

@dfolch dfolch commented Jul 10, 2015

This PR addresses #653. This change inserts zeros on the diagonal of a distance matrix instead of NANs.

@sjsrey
Copy link
Member

sjsrey commented Jul 10, 2015

@jlaura should have a look as I don't remember the original motivation for the nan fill?

@jGaboardi
Copy link
Member

As far as I can tell there is no benefit to have NaN instead of float(0.0) on a distance-based matrix.

  • J

On Jul 10, 2015, at 4:38 PM, Sergio Rey <notifications@github.commailto:notifications@github.com> wrote:

@jlaurahttps://github.com/jlaura should have a look as I don't remember the original motivation for the nan fill?


Reply to this email directly or view it on GitHubhttps://github.com//pull/660#issuecomment-120516354.

@ljwolf
Copy link
Member

ljwolf commented Jul 11, 2015

I can think of one for generic distance matrices.

Imagine where we make a network for potential spatial interaction situations where a point is a valid origin but not a destination.

In that case, the self-distance should not be 0, but np.nan. Right now, if we make them nan, we can't remask from zero. But, if nan, it's simple to use builtin masking tools to make nan into zeros.

In general distance matrices, we need to preserve a way to differentiate nonsymmetric origin-destinations. This commit does this. But, we need to remember this. This commit only affects symmetric matrices, which is the point of using zero and not nan. For a symmetric matrix, @jGaboardi is def right.

If zero masking is currently burning someone's use case, we should totally merge.

@ljwolf
Copy link
Member

ljwolf commented Feb 19, 2016

Also, was thinking about this the other day. For some distance matrix dist, it's very convenient to be able to call things like dist.min(axis=1) or np.argmin(dist, axis=1) to extract nearest neighbors from a pairwise distance matrix. Infilling diagonals with zeros removes this ability. Not sure if this is desirable.

@dfolch
Copy link
Member Author

dfolch commented Feb 19, 2016

These are good arguments for nan on the diagonal. I think that the rest of pysal assumes zeros on the diagonal, unless explicitly stated otherwise. My interpretation of the earlier design decision was that most of the downstream functions were expecting zeros (or some numeric value) in all slots, so that was the default. The function weights.util.insert_diagonal() was written for the non-typical cases. Sounds like the typical (or better) case for network is nan.

@dfolch dfolch closed this Feb 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants