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

Update LCF_graph docstring #7262

Merged
merged 2 commits into from Mar 10, 2024

Conversation

rossbar
Copy link
Contributor

@rossbar rossbar commented Jan 30, 2024

Use numpydoc docstring standard and fixup formatting/examples.

Use numpydoc docstring standard and fixup formatting/examples.
@rossbar rossbar force-pushed the doc/fix-lcf-notation-docstring branch from 97ed184 to e2e551b Compare March 5, 2024 18:35
Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

This is a big improvement! Thanks @rossbar!!
I think the parameter descriptions are really helpful. The only part I still have trouble with is the sentence describing how to decide which nodes are connected.

Comment on lines 72 to 74
For each node ``n_i``, cycling through the n-cycle a total of ``len(shift_list) * repeats``
with shift cycling through `shift_list` `repeat` s times connects ``n_i``
with ``n_i + shift mod n``.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is not clear to me -- though some of that may be because words and notation are defined in the parameters section below. Is there a way to describe what this will say before saying it? Or maybe turning the sentence around with "Each node n_i connects with n_i + shift mod n where shift is cycling through input shift_list ... "...

It would probably also help to say that all the nodes are the first N integers, with connections determined by relationships mod N. But I don't understand enough to describe that relationship yet. I understand much better than before though. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in the same boat - this was my feeble attempt at mashing together the existing description with what I could understand from reading the code. Let me take another stab incorporating these suggestions!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took your advice of flipping the sentence around which I agree improves the clarity. Whether or not it improves the situation enough is another question --- LMK what you think!

Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

Wow -- Very Nice!
I can understand it without standing on my head. :)
Thanks!

@dschult dschult merged commit 8f1e3a7 into networkx:main Mar 10, 2024
41 checks passed
@jarrodmillman jarrodmillman added this to the 3.3 milestone Mar 10, 2024
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
* Update lcf_graph docstring.

Use numpydoc docstring standard and fixup formatting/examples.

* Reword incorporating review suggestions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants