[Feat] Random expanders utilities

[fcrozatier]

Implements #6541

[dschult]

To get the tests working, you need to set up the import and tests so that this function doesn't impact the rest of networkx when numpy isn't even installed. You've done some of it, but

How to test functions/modules which use numpy/scipy/matplotlib, etc:

• Put the import statements inside the functions which use them. That delays the import until it is needed.
• In the tests, use np = pytest.importorskip("numpy"). Note that this function is pronounced "import-or-skip". If it can import numpy, it attaches that module to the name np. If it can't, all tests in this scope are skipped. Please this importorskip call so that it affects only tests that call a function involving numpy (or that use numpy for the test). The idea is to skip functions that will fail because numpy isn't available. Usually this is either near the start of a test module, near the start of a test function, or near the start of a test class. That will skip tests in that module, function or class respectively.
• Add a line to the networkx/conftest.py section which defines "needs_numpy" or" needs_...`.

Hopefully I have remembered everything. This should fix the CI failures due to missing numpy.

[fcrozatier]

Ok thanks for the help, hopefully it won't break the CI pipeline now

[dschult]

Could you wrap the comment lines (or remove words/length from them so that the file is within the 88 char style for the code? (<80 is better but you can overflow to 88 occasionally)

For the doc_strings, the first line is used as a short description on links in the documentation to the page for that function. So could you move the long first lines down, add a short one-liner as a doc_string heading followed by a blank line and then finally the longer (paragraph) description of what the function actually does.

Thanks for this!

[dschult]

Also, since this is a new function, can you please make the keyword arguments "keyword only" by adding a *, between the positional arguments and the keyword arguments?

[dschult]

Thanks for this contribution!
And sorry for the delay in our review.

I've got a few comments below -- many of which you could probably use the "batch commit" feature within the github interface -- where you commit the suggestions you "batch" all together as one commit. Remember to pull those changes to your local machine before making further changes. Some of the other comments will require you to make changes on your branch so if you prefer to make them all locally and then push them that is fine -- you don't have to use the github commit suggestions interface.

[fcrozatier]

@dschult Thanks for review I've implemented almost all changes, just not sure about scipy since we need to know for sure we computed the second biggest eigenvalue

[dschult]

Thanks for this!

Is the maybe_regular_expander function a helper function that should be made private?

Should random_regular_expander be called random_regular_expander_graph?

Could you add a paragraph to each doc_string briefly explaining what an expander graph is -- and a Ramanajan graph where you bring that term up.

I wonder if the whlie loops could become effectively infinite. Often we put a max_tries argument to avoid that, raising a NetworkXError if one is not found. If there's a reason that shouldn't be necessary here that's fine.

Because these involve random number generators, this should have a seed argument and a decorator like nx.utils.np_random_state to process the inoput and use the resulting input variable seed when calling any function with random aspects.

You'll need to connect these to the documentation with an entry in doc/reference/generators/expanders.rst. model it after paley_graph I guess.

[dschult]

Do you have some (one?) example network where you know from a trusted source /publication/worked it out what the result should be? That would help a lot in ensuring that it is working.

The scipy eigsh function does return the largest two eigenvalues when the right which option is specified. So I think we're good there.

[fcrozatier]

Thanks @rossbar for the review! I've added the seed pattern. Do you have any idea what's wrong with the failing test?

SyntaxError: invalid escape sequence '\l'

[dschult]

The invalid sequence comes from your docstring where the latex code is interreted as containing escaped special characters instead of as latex code. If you don't need special characters in your doc_string, take out the r in from of r""" at the start of the doc_string. If you do have a special character in there somewhere we will have to do something like double \\ in the latex part of the doc_string.

[dschult]

Ack -- I think I got it backwards... The doc_strings work with r""" and single \ in the latex code.
So, instead of replacing r""" and adding double backspaces, it is better to add the r to the is_regular_expander doc_string. Sorry for the confusion. Now the doc_strings latex code looks ugly.

To fix it, replace all the double backslashes with single backslashes.
Then add r""" at the start of the doc_string for is_regular_expander.
I can fix it if you prefer.

[fcrozatier]

Ok this is confusing me now it breaks the tests again. Can you fix this?
This also used to work, did something change with the latex parsing?

[dschult]

You are correct -- the tests used to work. But we were getting warnings at import time, so a new test was added last week to check for warnings at import time -- and you were unfortunate enough to get caught by it. Sorry about that. I have got it working by making every doc_string that has latex in it a "raw string" (using r""" at the beginning).

I think this is ready to go, but I've only quickly checked the resolutions of @rossbar's suggestions.

[fcrozatier]

Thank you for the fix!

[rossbar]

Thanks @fcrozatier , I made a few very minor tweaks to the docstring but otherwise LGTM!

[MridulS]

Hmm, the random test suite cron job is failing as the seed is tripping up UNEXPECTED EXCEPTION: NetworkXError('Too many iterations in maybe_regular_expander') error. Should we set a seed that we know will pass?