Skip to content

Add c.network.list() #604

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Add c.network.list() #604

wants to merge 13 commits into from

Conversation

mbojan
Copy link
Member

@mbojan mbojan commented May 9, 2025

c.f. #603

@mbojan mbojan requested review from krivit and removed request for krivit May 9, 2025 13:43
@mbojan mbojan marked this pull request as draft May 9, 2025 13:58
@mbojan
Copy link
Member Author

mbojan commented May 9, 2025

Drafty still as I did something stupid... I'll add a test or two as well.

Comment on lines 13 to 17
# Check a list elements using all.equal or identical
check_list <- function(lst, fun = all.equal) {
r <- vapply(lst[-1], function(x) isTRUE(fun(x, lst[[1]])), logical(1))
all(r)
}
Copy link
Member

Choose a reason for hiding this comment

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

This function is similar to statnet.common::all_identical(). Would you mind contributing it (or a version of it) there? Also, it might be worth benchmarking, because the all_identical()'s for loop seems to perform about 4 times faster.

Copy link
Member Author

@mbojan mbojan May 13, 2025

Choose a reason for hiding this comment

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

Ha! I missed that. all_identical() is more efficient for sure, because it exits as soon as an element different from the first is found. I'll use it and drop this.

Perhaps I'll add sth like all_equal() that will work in a similar manner but use all.equal(). Such algo might be quirky though as I think of it, because, in general, all.equal() is not transitive due to numerical tolerance: for some c(x,y,z) all.equal(x, y) and all.equal(x, y) might be true but all.equal(x, z) might be false...

@mbojan
Copy link
Member Author

mbojan commented May 13, 2025

This should pass the checks as soon as statnet/statnet.common#34 is merged.

@krivit
Copy link
Member

krivit commented May 19, 2025

Actually, could you please put the c.network.list() into R/network.list.R?

@krivit krivit force-pushed the i603-c-networklist branch from 89f1fb9 to 19949c3 Compare May 20, 2025 02:35
@krivit krivit marked this pull request as ready for review May 20, 2025 02:35
@krivit
Copy link
Member

krivit commented May 20, 2025

@mbojan , I've collapsed the changes to avoid having to merge with a file that no longer exists. One last item is perhaps a unit test or two?

@mbojan
Copy link
Member Author

mbojan commented May 20, 2025

Ok, will do. I was wondering where the conflict came from.

@krivit
Copy link
Member

krivit commented May 21, 2025

Ok, will do. I was wondering where the conflict came from.

I went through and merged a bunch of files in the R directory, which included all the files pertaining to network.list and its methods. In order to keep the version history, I needed to do it in a roundabout way (essentially create a branch for each source file, rename it to a temporary file in that branch, then merge the branches onto each other, and rename the temporary file (which will now contain all the files' contents) into its final name).

@mbojan mbojan force-pushed the i603-c-networklist branch from 19949c3 to 84e2c96 Compare May 22, 2025 15:49
@mbojan
Copy link
Member Author

mbojan commented May 22, 2025

I rebased against the current master and will add a unit test or two.

@mbojan
Copy link
Member Author

mbojan commented May 23, 2025

Tests added and conflicts resolved.

response = attr(dots[[1]], "response"),
stats = structure(
do.call("rbind", map(dots, ~attr(.x, "stats"))),
monitored = do.call("c", map(dots, ~ attr(attr(.x, "stats"), "monitored")))
Copy link
Member

Choose a reason for hiding this comment

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

The "monitored" attribute has an element for each statistic element, not each realisation, so it should be checked for consistency, not concatenated.

control = attr(dots[[1]], "control"),
response = attr(dots[[1]], "response"),
stats = structure(
do.call("rbind", map(dots, ~attr(.x, "stats"))),
Copy link
Member

Choose a reason for hiding this comment

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

This does not work for simulate(simplify = FALSE), because then attr(., "stats") is an mcmc.list, not a matrix.

@krivit krivit force-pushed the i603-c-networklist branch from ff7681f to 887271b Compare May 28, 2025 13:49
@krivit krivit force-pushed the i603-c-networklist branch from 887271b to 04baa72 Compare May 30, 2025 11:34
@mbojan
Copy link
Member Author

mbojan commented Jun 3, 2025

Please don't rebase until I resolve all the issues.

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.

2 participants