Skip to content

Make library SPEC-7 compliant #28

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

Merged
merged 13 commits into from
Dec 29, 2024
Merged

Make library SPEC-7 compliant #28

merged 13 commits into from
Dec 29, 2024

Conversation

eliotwrobson
Copy link
Owner

See title, resolves #21

@eliotwrobson eliotwrobson added the enhancement New feature or request label Dec 14, 2024
@Kai-Striega
Copy link

@eliotwrobson technically, this looks good.

An issue that I have is that I don't really know how to treat sequences of RNG/seed instances. For example, you can now define something as follows:

model, _ = networkx_to_ic_model(graph, activation_prob=0.2, rng=[1, 42])

What does it mean to have multiple RNG seeds?

Should the model be run for each seed? e.g. rather than running:

n_sim = 1_000
total = 0.0

for _ in range(n_sim):
    # Resetting the model doesn't change the initial seed set used.
    model.reset_model()
    model.advance_until_completion()
    total += model.get_num_activated_nodes()

avg = total / n_sim

Should I now run something like the following:

# Note this code could be wrong, I haven't tested it, but it gets the idea across (hopefully)
rng_seeds = [x for x in range(n_sims)]
models, _ = networkx_to_ic_model(graph, activation_prob=0.2, rng=rng_seeds)
nodes = (model.get_num_activated_nodes() for model in models)
avg = sum(nodes) / n_sims

This is a domain question, and I can't answer it for you. But you should probably have a think about it.

@eliotwrobson
Copy link
Owner Author

@Kai-Striega thanks for the feedback! I see what you mean, I'll make a couple of small changes and get back to you.

@eliotwrobson
Copy link
Owner Author

Following up on the previous comment, the goal of the rng seeding should be to guarantee deterministic output from model execution, even after multiple runs (you want to seed once and run 1000 times). I don't think there's a use case for wanting to equip the model with a separate random seed each run, as this would make 1000 consecutive runs very slow because of rng reseeding.

However, the way the models are written means that the desired behavior is already the current behavior after multiple runs (i.e., the third run after having the first initial seed is always the same, not just the first). To assert this behavior, I've added a test case that runs the model multiple times after a reseeding and asserts that each run after reseeding activates the same set of nodes. See the code below:

https://github.com/eliotwrobson/CyNetDiff/pull/28/files#diff-15c4ec0cade368b4c242fd6f38802f54c06c94ad25b33d9143979e85d2bdad00R191-R229

Since @Kai-Striega had no other issues, will go ahead and merge this PR, and we can call this feature completed!

@eliotwrobson eliotwrobson merged commit 84005e6 into main Dec 29, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt SPEC-7 Standard
2 participants