igraph / igraph

Library for the analysis of networks
https://igraph.org
GNU General Public License v2.0
1.75k stars 405 forks source link

HRG functions should not require attribute handler #2314

Closed szhorvat closed 1 year ago

szhorvat commented 1 year ago

The HRG functionality currently requires an attribute handler. Specifically, igraph_hrg_dendrogram() adds a vertex attribute called probability.

All analysis functions should work without an attribute handler, and should return data output arguments instead of attributes. This should be done in the HRG functions as well.

Since this is a breaking change, targeting for 0.11.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

ntamas commented 1 year ago

Okay, so an obvious modification of igraph_hrg_dendrogram() would be to have a signature like this:

igraph_error_t igraph_hrg_dendrogram(igraph_t *graph, const igraph_hrg_t *hrg, igraph_vector_t *prob);

But, this begs the question: should prob be an already-initialized vector that the function will resize and fill appropriately, or should it be an uninitialized vector that the function initializes internally? Note that graph is an uninitialized igraph object so the function acts as a "constructor" in some sense.

Also, since igraph_hrg_dendrogram is essentially a constructor, it might be better to call it igraph_from_hrg_dendrogram()? This would allow us to remain backward compatible: we can make a new function with the prob argument, deprecate igraph_hrg_dendrogram() and make it simply call igraph_from_hrg_dendrogram() internally.

szhorvat commented 1 year ago

But, this begs the question: should prob be an already-initialized vector that the function will resize and fill appropriately, or should it be an uninitialized vector that the function initializes internally?

If I saw this function signature without having read the docs, I'd assume that prob needs to be already initialized. But that's just me.

Arguments for requiring an initialized vector: