ENH : Provide non-normalized and normalized directed laplacian matrix calculation #7199

smokestacklightnin · 2023-12-31T04:52:57Z

This pull request addresses Issue #7148

Here, I provide both normalized and non-normalized graph Laplacian as well as improve the documentation for them.

I welcome any comments or questions

…matrix`

dschult

Nicely done and with good tests.
And thanks for that reference.

For the docs, can you add the reference as part of that?
Maybe use the same example as is done for the undirected case so people can compare. If you have other better ideas please use them instead of my suggestions. :)

I don't think the current example should have the print statement. But the printing of numpy arrays in examples has been problematic in the past, so if removing that print function call makes it fail, leave it in obviously.

Thanks very much!!

…al_laplacian_matrix` in docstring of `laplacian_matrix`

…etric output to `directed_combinatorial_laplacian_matrix` and `directed_laplacian_matrix`.

… along with associated references Also removed `print` statement from example in `laplacian_matrix`

smokestacklightnin · 2024-01-04T03:44:33Z

@dschult I believe I've done everything you suggested in the Issue and your comments above.

I want to confirm that I understand your suggestion below:

In the doc_strings for those functions (or maybe in the module level doc_string, or maybe both: We should describe how all these relate. (The non-normalized version is produced by the main function, and normalized versions for undirected and the two normalizations for the directed case are provided by the other functions.)

Is something like this, this, and this what you had in mind?

networkx/linalg/laplacianmatrix.py

dschult

I like the tests, the addition of normalized_laplacian_matrix handling of DiGraph and the content of the doc_strings. The only things left as far as I can see involve formatting of doc_strings and making the examples differ from the test graphs. If this is too much trouble let me know as it's more formatting than coding and I know the NX formatting.

It looks like removing the "print" from the doc_string test did cause one test failure. Looks like the repr can vary from system to system, while str is consistent. So could we try putting the print back in? print(nx.laplacian_matrix(G).toarray())
The doc_strings should be wrapped to ~85 chars like the code. This can be a pain so if it doesn't fit, go over the limit, but if it is easy make a line break. Thsi doesn't change the html docs, but it does make it more uniform when reading the docs in the code.
Could we make the directed example one node smaller than the test graph (which comes fro the article)? For example, remove node 6. Or if you prefer 2 and 6? My goal is to make it easier to read the example, even if the resulting graph is not as good of a test of the algorithm.
Could we put the set_printoptions and comment on a separate line from the import of numpy?

I like the moving of the 1/2 in the math expressions. I like the tests. I like the expansion to the normalized_laplacian_matrix case. It's kind of amazing that we already had two normalized directed functions and we still didn't cover the basic extension of the undirected normalized definition. It just shows that there are many more possibilities in directed graphs than undirected graphs.

Thanks for this!

Also -- the dev tests might not pass due to new releases of pytest. We're working on that. After it is fixed on main we can merge this PR with main and they should pass.

networkx/linalg/laplacianmatrix.py

smokestacklightnin · 2024-01-06T03:17:17Z

It looks like removing the "print" from the doc_string test did cause one test failure. Looks like the repr can vary from system to system, while str is consistent. So could we try putting the print back in? print(nx.laplacian_matrix(G).toarray())

Done!

The doc_strings should be wrapped to ~85 chars like the code. This can be a pain so if it doesn't fit, go over the limit, but if it is easy make a line break. Thsi doesn't change the html docs, but it does make it more uniform when reading the docs in the code.

Done!

Could we make the directed example one node smaller than the test graph (which comes fro the article)? For example, remove node 6. Or if you prefer 2 and 6? My goal is to make it easier to read the example, even if the resulting graph is not as good of a test of the algorithm.

Done!

Could we put the set_printoptions and comment on a separate line from the import of numpy?

Done!

All the tests pass now. Ready for review an merge!

rossbar

Generally LGTM, thanks @smokestacklightnin . There's an issue with line length that should automatically be corrected by the linter, but which is currently not being handled correctly for whatever reason.

That issue should be handled separately though - for this PR, I think we can just go in and manually fix it prior to merging.

rossbar · 2024-01-06T18:30:28Z

networkx/linalg/laplacianmatrix.py

+
+All calculations here are done using the out-degree. For Laplacians using in-degree, us `G.reverse(copy=False)` instead of `G`.
+
+The `laplacian_matrix` function provides an unnormalized matrix, while `normalized_laplacian_matrix`, `directed_laplacian_matrix`, and `directed_combinatorial_laplacian_matrix` are all normalized.


Did our linting rules change? These lines are clearly greater than 100 chars so I would've expected CI to complain about it. @MridulS any ideas?

Should I go ahead and just fix the line lengths manually now?

I don't think our lint checks line lengths for comments. But I could be wrong.

Yes, @smokestacklightnin it would help to wrap the comment lines to about 80 chars (less than 88 for sure). There are others below...

dschult

Thanks very much for this.
It looks good to me... :)

rossbar · 2024-01-09T17:42:52Z

Excellent, thanks again @smokestacklightnin !

… calculation (networkx#7199) Adds functionality for directed laplacian matrices * Remove the `not_implemented_for("directed")` decorator on `laplacian_matrix` * Add tests for 'laplacian_matrix` as applied to directed graphs * Add pointers to `directed_laplacian_matrix` and `directed_combinatorial_laplacian_matrix` in docstring of `laplacian_matrix` * Allow DiGraphs in `normalized_laplacian_matrix`

smokestacklightnin added 2 commits December 30, 2023 18:11

Remove the not_implemented_for("directed") decorator on `laplacian_…

d78c98a

…matrix`

Add tests for 'laplacian_matrix` as applied to directed graphs

1a16129

smokestacklightnin changed the title ~~Provide non-normalized directed laplacian matrix calculation~~ DOC ENH : Provide non-normalized directed laplacian matrix calculation Dec 31, 2023

dschult added the type: Enhancements label Dec 31, 2023

dschult reviewed Dec 31, 2023

View reviewed changes

smokestacklightnin added 8 commits December 31, 2023 21:04

Add pointers to directed_laplacian_matrix and `directed_combinatori…

0b57164

…al_laplacian_matrix` in docstring of `laplacian_matrix`

Allow DiGraphs in normalized_laplacian_matrix

7e7ba87

Add test for laplacian_matrix for MultiDiGraphs

8023e80

Add tests for normalized_laplacian_matrix for directed graphs

3252e74

Add pointers to other related Laplacian functions

32e7e3f

Add minor mathematical notational changes and a quick note about symm…

b885c83

…etric output to `directed_combinatorial_laplacian_matrix` and `directed_laplacian_matrix`.

Add note about using in-degree instead of out-degree

ef90143

Add examples for laplacian_matrix and normalized_laplacian_matrix…

c0622e4

… along with associated references Also removed `print` statement from example in `laplacian_matrix`

smokestacklightnin marked this pull request as ready for review January 4, 2024 03:22

Add comments about normalization

1ef1823

smokestacklightnin requested a review from dschult January 4, 2024 03:44

smokestacklightnin commented Jan 4, 2024

View reviewed changes

networkx/linalg/laplacianmatrix.py Show resolved Hide resolved

dschult reviewed Jan 4, 2024

View reviewed changes

networkx/linalg/laplacianmatrix.py Show resolved Hide resolved

smokestacklightnin added 3 commits January 5, 2024 23:41

Simplify example for laplacian_matrix

b8edb47

Simplify example for normalized_laplacian_matrix

42ea490

Added print statement to example

e7a22c8

smokestacklightnin requested a review from dschult January 6, 2024 03:18

smokestacklightnin changed the title ~~DOC ENH : Provide non-normalized directed laplacian matrix calculation~~ DOC ENH : Provide non-normalized and normalized directed laplacian matrix calculation Jan 6, 2024

smokestacklightnin changed the title ~~DOC ENH : Provide non-normalized and normalized directed laplacian matrix calculation~~ ENH : Provide non-normalized and normalized directed laplacian matrix calculation Jan 6, 2024

rossbar approved these changes Jan 6, 2024

View reviewed changes

smokestacklightnin added 2 commits January 8, 2024 18:55

Shorten lines to be less than or equal to 80 characters wide

ac0a073

Make parentheses fit what they are around

052316f

dschult approved these changes Jan 9, 2024

View reviewed changes

rossbar merged commit e36da1d into networkx:main Jan 9, 2024
39 checks passed

jarrodmillman added this to the 3.3 milestone Jan 9, 2024

rossbar mentioned this pull request Jan 9, 2024

Provide non-normalized directed laplacian matrix calculation #7148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH : Provide non-normalized and normalized directed laplacian matrix calculation #7199

ENH : Provide non-normalized and normalized directed laplacian matrix calculation #7199

smokestacklightnin commented Dec 31, 2023 •

edited

dschult left a comment

smokestacklightnin commented Jan 4, 2024 •

edited

dschult left a comment •

edited

smokestacklightnin commented Jan 6, 2024 •

edited

rossbar left a comment

rossbar Jan 6, 2024

smokestacklightnin Jan 6, 2024

dschult Jan 8, 2024

smokestacklightnin Jan 8, 2024

dschult left a comment

rossbar commented Jan 9, 2024


		All calculations here are done using the out-degree. For Laplacians using in-degree, us `G.reverse(copy=False)` instead of `G`.

		The `laplacian_matrix` function provides an unnormalized matrix, while `normalized_laplacian_matrix`, `directed_laplacian_matrix`, and `directed_combinatorial_laplacian_matrix` are all normalized.

ENH : Provide non-normalized and normalized directed laplacian matrix calculation #7199

ENH : Provide non-normalized and normalized directed laplacian matrix calculation #7199

Conversation

smokestacklightnin commented Dec 31, 2023 • edited

dschult left a comment

Choose a reason for hiding this comment

smokestacklightnin commented Jan 4, 2024 • edited

dschult left a comment • edited

Choose a reason for hiding this comment

smokestacklightnin commented Jan 6, 2024 • edited

rossbar left a comment

Choose a reason for hiding this comment

rossbar Jan 6, 2024

Choose a reason for hiding this comment

smokestacklightnin Jan 6, 2024

Choose a reason for hiding this comment

dschult Jan 8, 2024

Choose a reason for hiding this comment

smokestacklightnin Jan 8, 2024

Choose a reason for hiding this comment

dschult left a comment

Choose a reason for hiding this comment

rossbar commented Jan 9, 2024

smokestacklightnin commented Dec 31, 2023 •

edited

smokestacklightnin commented Jan 4, 2024 •

edited

dschult left a comment •

edited

smokestacklightnin commented Jan 6, 2024 •

edited