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

Random models faster #213

Merged
merged 10 commits into from
Nov 23, 2022
Merged

Random models faster #213

merged 10 commits into from
Nov 23, 2022

Conversation

maximelucas
Copy link
Collaborator

Speed up of random_hypergraph() and random_simplicial_complex().
Running

%%timeit
N = 100 
ps = [0.00001, 0.000001, 0.00000000001]
H = xgi.random_hypergraph(N, ps)

we had

# -> 225 ms ± 2.04 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

and now

# -> 154 ms ± 483 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

Because I switched to the numpy random number generator, the tests are not happy anymore. Not managing to make the seed work correctly so far.

@@ -247,7 +248,7 @@ def dcsbm_hypergraph(k1, k2, g1, g2, omega, seed=None):
return H


@py_random_state("seed")
@np_random_state(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should either be np_random_state(2) or np_random_state("seed"). Are those not working?

Copy link
Collaborator Author

@maximelucas maximelucas Nov 7, 2022

Choose a reason for hiding this comment

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

With np_random_state(2) and np_random_state("seed") the error seems slightly different:

______________________________ test_compute_kuramoto_order_parameter _______________________________

    @pytest.mark.slow
    def test_compute_kuramoto_order_parameter():
>       H1 = xgi.random_hypergraph(100, [0.05, 0.001], seed=0)

tests/dynamics/test_synchronization.py:11:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
xgi/utils/decorators.py:578: in func
    return argmap._lazy_compile(__wrapper)(*args, **kwargs)
<class 'xgi.utils.decorators.argmap'> compilation 4:4: in argmap_random_hypergraph_1
    ???
xgi/generators/nonuniform.py:285: in random_hypergraph
    rng = np.random.default_rng(seed=seed)
_generator.pyx:4849: in numpy.random._generator.default_rng
    ???
_pcg64.pyx:121: in numpy.random._pcg64.PCG64.__init__
    ???
bit_generator.pyx:517: in numpy.random.bit_generator.BitGenerator.__init__
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   ???
E   TypeError: SeedSequence expects int or sequence of ints for entropy not RandomState(MT19937)

bit_generator.pyx:299: TypeError

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well damn. Yeah we seriously have to refactor the random state stuff. This was copy/pasted from NX, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pretty much yea

Copy link
Collaborator

@nwlandry nwlandry Nov 16, 2022

Choose a reason for hiding this comment

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

What would happen if we simply removed the decorators for random numbers from XGI? It seems a bit overkill, but perhaps I'm underthinking it. The tests runs successfully for me when I remove the decorator (except for the Kuramoto tests which can be easily fixed). Either way NetworkX is deprecating preserve_random_state and random_state in v3.0 anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I removed decorators in PR #227 so you can see what I mean. IDK if we decide to do that, but IMO it simplifies things a bit.

Copy link
Collaborator

@leotrs leotrs Nov 17, 2022

Choose a reason for hiding this comment

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

The decorators are there so that the function can accept either a seed number or an object that generates a seed. The former is good for users and the latter is good for testing, and it also supports newer numpy syntax for random stuff.

Having said that, I never liked that we just depended on this black box module that was copy-pasted from NX. So I suggest we remove those decorators for now and make it a priority issue to figure out how to support modern numpy random stuff.*

* By "modern random stuff" I mean the attached, taken from here.
2022-11-17-074228_936x157_scrot

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great so for now I'll just merge Nicho's decorator PR and into this branch and use the numpy random generators for this PR.

@nwlandry
Copy link
Collaborator

@maximelucas I did a check comparing the number of edges using the old method versus the new method and verified that the number of edges is preserved, but as you anticipated, because we are switching from a random method to a numpy random method, the specific edges aren't the same.

@maximelucas
Copy link
Collaborator Author

maximelucas commented Nov 23, 2022

Great thanks @nwlandry! Had started locally but never got round to finishing it :)
Updated the last one now, and fixed outputs and added a test.

@maximelucas maximelucas marked this pull request as ready for review November 23, 2022 18:10
Copy link
Collaborator

@nwlandry nwlandry left a comment

Choose a reason for hiding this comment

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

Looks great!

@maximelucas maximelucas merged commit d12bd51 into main Nov 23, 2022
@maximelucas maximelucas deleted the random-models-faster branch November 23, 2022 18:49
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.

None yet

3 participants