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

feat: sample_chung_lu() #1416

Merged
merged 4 commits into from
Jun 24, 2024
Merged

feat: sample_chung_lu() #1416

merged 4 commits into from
Jun 24, 2024

Conversation

szhorvat
Copy link
Member

Closes #1369.

Translating the documentation was a lot of work. If there are minor stylistic issues to fix, I'd appreciate it if someone could take this over and fix those. Let's try to have this in the next release please.

@szhorvat szhorvat requested a review from krlmlr June 22, 2024 16:37
Copy link
Contributor

aviator-app bot commented Jun 22, 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.

@szhorvat szhorvat added this to the 2.0.4 milestone Jun 22, 2024
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks! I glimpsed over the text and the autogenerated code, looks good. Would you add the ellipsis?

R/aaa-auto.R Outdated
@@ -391,6 +391,25 @@ simple_interconnected_islands_game_impl <- function(islands.n, islands.size, isl
res
}

chung_lu_game_impl <- function(out.weights, in.weights=NULL, loops=TRUE, variant=c("original", "maxent", "nr")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this?

Suggested change
chung_lu_game_impl <- function(out.weights, in.weights=NULL, loops=TRUE, variant=c("original", "maxent", "nr")) {
chung_lu_game_impl <- function(out.weights, ..., in.weights=NULL, loops=TRUE, variant=c("original", "maxent", "nr")) {

Copy link
Member Author

@szhorvat szhorvat Jun 24, 2024

Choose a reason for hiding this comment

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

No, definitely not. The ellipsis should come after in.weights, not before. Out- and in-weights go together and users shouldn't be forced to spell out the name of these parameters.

#' degree(sample_chung_lu(c(1, 3, 2, 1), c(2, 1, 2, 2), variant = "maxent"), mode='out')
#' ))
#' @export
sample_chung_lu <- chung_lu_game_impl
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should forward or alias. Both have advantages.

@szhorvat
Copy link
Member Author

Would you add the ellipsis?

I think the ellipsis makes sense like this:

function(out.weights, in.weights=NULL, ..., loops=TRUE, variant=c("original", "maxent", "nr"))

I can add it, but I need some help here. Notice that there are two functions on the same doc page (as with all sample_ and make_ functions), and one of them already has a ..., which is already documented:

image
image

Is this going to be an issue? What should I do here? Just keep the docs as they are even though it is slightly confusing?

@szhorvat
Copy link
Member Author

@krlmlr I pushed a commit adding the ellipsis to illustrate. This is what the docs look like now, with the ... moved to a confusing 3rd place. In reality the doc text refers to chung_lu(), not to sample_chung_lu(). Let me know how to proceed.

image

@krlmlr
Copy link
Contributor

krlmlr commented Jun 24, 2024

Good point. I pushed a proposal, it's a bit of work but solves the issue at hand and also offers autocomplete for chung_lu() .

How is chung_lu() used anyway?

@szhorvat
Copy link
Member Author

How is chung_lu() used anyway?

It's used with sample_(). The make_() doc page has examples. One can add constructor modifiers: https://r.igraph.org/reference/index.html#constructor-modifiers I don't have much direct experience with using these.

@szhorvat
Copy link
Member Author

I think this is done and ready to go, right?

@krlmlr
Copy link
Contributor

krlmlr commented Jun 24, 2024

Thanks!

@szhorvat
Copy link
Member Author

Thanks @krlmlr !

@aviator-app aviator-app bot merged commit 7f69743 into main Jun 24, 2024
11 checks passed
@aviator-app aviator-app bot deleted the feat/chung_lu branch June 24, 2024 10:33
@szhorvat
Copy link
Member Author

I forgot to mark it as experimental, which is necessary since it's experimental in C. I pushed this change to main directly: 4a3983d Are you okay with this?

@krlmlr
Copy link
Contributor

krlmlr commented Jun 24, 2024

Works for me, as long as the main branch stays green 🙃

@szhorvat
Copy link
Member Author

OK, I'll continue to make simple doc changes on main, for efficiency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add igraph_chung_lu_game() to R
2 participants