Bonpow weights

[davidskalinder]

This should partially fix #903 (i.e., it should allow the R code to handle weights, but not provide a full C implementation). I tried to keep all changes as minimal as possible and reuse logic from existing functions, especially for the behavior of the weights arg.

[davidskalinder]

Yes, I think you're quite right on all counts -- I stupidly didn't test all the weights conditions carefully enough and so missed the underlying cause of all of this, which is that as_adj() wants the weight attribute as a string whereas both strength() and most of the other centrality functions (correctly) want it as a vector. My apologies for taking up your time with a lousy PR.

I could implement the workaround you suggest of using the weights logic (and documentation) from alpha_centrality(), kludgey though it may be. (Indeed I think I might have found a bug there, too -- if I can confirm that then I'll report it separately.)

However it sounds like that workaround does create the problem that once as_adj()'s behavior changes, power_centrality()'s should too (i.e., they will both switch from accepting strings as weights to accepting vectors as weights). If you guys would prefer to avoid creating that behavior that will soon change, then maybe it's best to fall back on the strategy of simply declaring in the documentation that power_centrality() doesn't support weights until it can do it with a vector argument?

So I think the options are for me to:

• Apply the weights logic from alpha_centrality() to power_centrality().
• Leave power_centrality() without the ability to handle weights, and mention this in the docs.

Let me know which one you prefer and I can resubmit.

[szhorvat]

My apologies for taking up your time with a lousy PR.

Thanks for submitting the PR! It would indeed be nice to fix this eventually, and it's already drawing attention to issues with the adj. mat. conversion functions, which is a good thing.

(Indeed I think I might have found a bug there, too -- if I can confirm that then I'll report it separately.)

Please do!

Let me know which one you prefer and I can resubmit.

We would need to wait for input from @krlmlr for this.

Personally I would suggest keeping this PR open for the moment, and finalizing it once the 0.10 update branch is in a good enough shape. That's going to be the most robust solution that takes the least amount of work. But I'm not sure when the 0.10 work will be in a good enough shape that it's ready for feature contributions like this one.

[aviator-app[bot]]

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes. Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was closed without merging. If you still want to merge this PR, re-open it.

---

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

[krlmlr]

We're on igraph/C 0.10 now, would you like to take another stab?

[davidskalinder]

We're on igraph/C 0.10 now, would you like to take another stab?

Ah, cool, thanks! I'll have a look and see if that's something I can help with.

[davidskalinder]

Hmm, looking at this again, I think the real issue is that the direction of this PR depends on #906, which I don't think has been resolved yet. As far as I can tell, as_adjacency_matrix() still can't accept a weights vector (though it looks like the underlying function get.adjacency.dense() now can). So as it stands, power_centrality() would still have to pass weights to as_adjacency_matrix() as an edge attribute name and to strength() as a vector.

So I think the options are still much as before (expanded here for clarity):

1. Wait until #906 has been resolved and then have power_centrality() accept a weights vector only.
2. If we want to improve things before that, we could do one of the following:
   1. Apply the kludgey weights logic from alpha_centrality() (with changes to handle #915) to power_centrality() now.
   2. Leave power_centrality() without the ability to handle weights for now, and mention this in the docs.

I might be overlooking something however; @szhorvat might have a better idea how these components should fit together?

[krlmlr]

Thanks. I don't mind waiting, unless a kludge would solve a use case now. We might need a few more months to actually resolve the adjacency matrix issue linked above.

[krlmlr]

Thank you for your contribution. Closing for now, will wait until after https://github.com/igraph/rigraph/issues/906. Adding a note to the motivating issue.