Random models faster #213
Random models faster #213
Conversation
xgi/generators/nonuniform.py
Outdated
| @@ -247,7 +248,7 @@ def dcsbm_hypergraph(k1, k2, g1, g2, omega, seed=None): | |||
| return H | |||
|
|
|||
|
|
|||
| @py_random_state("seed") | |||
| @np_random_state(0) | |||
There was a problem hiding this comment.
This should either be np_random_state(2) or np_random_state("seed"). Are those not working?
There was a problem hiding this comment.
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: TypeErrorThere was a problem hiding this comment.
Well damn. Yeah we seriously have to refactor the random state stuff. This was copy/pasted from NX, no?
There was a problem hiding this comment.
Pretty much yea
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@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. |
|
Great thanks @nwlandry! Had started locally but never got round to finishing it :) |
Speed up of
random_hypergraph()andrandom_simplicial_complex().Running
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.