Skip to content

feat: rename sample_degseq() method and add the edge.switching.simple method#1376

Merged
aviator-app[bot] merged 9 commits intomainfrom
sample_degseq
May 24, 2024
Merged

feat: rename sample_degseq() method and add the edge.switching.simple method#1376
aviator-app[bot] merged 9 commits intomainfrom
sample_degseq

Conversation

@maelle
Copy link
Copy Markdown
Contributor

@maelle maelle commented May 23, 2024

Fix #876

See @szhorvat, a new feature exposed after all the boring refactoring PRs. 😁 😇 Some questions

  • where do the numbers corresponding to the method come from? I assumed the new one was 4.
  • what could be a better test for the new method?

@maelle maelle requested a review from szhorvat May 23, 2024 12:23
@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented May 23, 2024

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 merged using Aviator.


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.

method <- igraph.match.arg(method)

if (method == "simple") {
lifecycle::deprecate_warn("2.0.4", "sample_degseq(method = 'must be configuration instead of simple')")
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hesitate between "warn" and "soft".

@szhorvat
Copy link
Copy Markdown
Member

where do the numbers corresponding to the method come from? I assumed the new one was 4.

For enum values in general, you need to look in include/igraph_constants.h in the C core:

https://github.com/igraph/igraph/blob/master/include/igraph_constants.h#L94

Enums in C are "named values" represented internally as integers. The numbers are integers. Notice that the first value is explicitly assigned 0. Anything after that that does not have an explicit assignment increments by 1. So yes, the new values is 4.

In the majority of cases we start at 0, but there are a handful of exceptions, so it's good to have a look at the sources.

what could be a better test for the new method?

Checking that the degree sequence of the result is what we input is the best we can do here. But your test has a problem: you generate an undirected graph then you use both in- and out-degrees, which are for directed graphs. Add directed=T in sample_gnm.

It might be useful, but not essential, to also test the undirected case, with an undirected starting point and undirected degrees.

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented May 23, 2024

It might be useful, but not essential, to also test the undirected case, with an undirected starting point and undirected degrees.

Do you have an example at hand?

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented May 23, 2024

I made edits @szhorvat. Thanks for your feedback!

@szhorvat
Copy link
Copy Markdown
Member

Do you have an example at hand?

  • Generate a directed=FALSE graph
  • Take degrees(graph, mode="all")
  • Use this as a single input to sample_degseq()
  • Check that the result has the same degrees, again with degrees(graph, mode="all")

@szhorvat
Copy link
Copy Markdown
Member

Could you please update the docs by lifting the descriptions of the five methods from the C docs here? https://igraph.org/c/html/latest/igraph-Generators.html#igraph_degree_sequence_game The current R docs have misleading statements. It's quickest to copy-paste the method description from C, and fix up the math so it actually renders are proper math.

@szhorvat
Copy link
Copy Markdown
Member

szhorvat commented May 23, 2024

I improved the C docs a bit in this commit: igraph/igraph@36a707a

@szhorvat
Copy link
Copy Markdown
Member

I made some more (independent) doc updates, so I'll just leave this link here instead of linking to comments:

https://github.com/igraph/igraph/blob/master/src/games/degree_sequence.c#L671

These updates did not affect the text I suggested you copy, but there's additional information there that might be worth copying (such as the "see also" section).

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented May 23, 2024

Is this PR good as is? I should put #1317 nearer the top of my list 😉

@szhorvat
Copy link
Copy Markdown
Member

Yes, let's merge this. Further doc improvements are always possible, and should not block this PR.

@aviator-app aviator-app bot merged commit 5ac3502 into main May 24, 2024
@aviator-app aviator-app bot deleted the sample_degseq branch May 24, 2024 07:26
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sample_degseq() method parameter values should be renamed to match C core

2 participants