normalization added #176

mhajij · 2023-06-25T12:32:03Z

often in DL computations normalization must be added to matrices for computations. This file is a collection of popular methods typically used in a DL setting.

codecov · 2023-06-26T05:41:23Z

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (a2808f2) 76.09% compared to head (396f70f) 96.87%.
Report is 410 commits behind head on main.

Files	Patch %	Lines
toponetx/utils/normalization.py	83.03%	19 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #176       +/-   ##
===========================================
+ Coverage   76.09%   96.87%   +20.77%     
===========================================
  Files          22       32       +10     
  Lines        2460     3518     +1058     
===========================================
+ Hits         1872     3408     +1536     
+ Misses        588      110      -478

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

toponetx/utils/normalization.py

michaelschaub · 2023-06-26T10:58:09Z

toponetx/utils/normalization.py

+    return out
+
+
+def kipf_adjacency_matrix_normalization(


I think we should use descriptive names for these functions (in that the names should indicate what the function does).

Something like:
normalized_symmetric_adjacency_matrix

is what is computed here. It is true (and perhaps) interesting that this is used in Kipf's work, but it is not an invention of that paper as well..
The information where this normalization is used could be provided in the function description with a reference. Examples: Kipf... [REF]

Second, it would be good to have all names consistent, atm we have things like

normalize_x_laplacian (verb first), and ..._adjacency_matrix_normalization.

We should decide for one style and apply consistently, Perhaps something like:

compute_xy_normalized_zmatrix ??

@michaelschaub do you know a better reference for the above normalization ?

Also, do you have a ref for D^{-1}A normalization?

please let me know if you like the current naming convection-I can change it again if you have something better in mind.

toponetx/utils/normalization.py

ffl096 · 2023-06-27T08:25:05Z

toponetx/__init__.py

+    _compute_D1,
+    _compute_D2,
+    _compute_D3,
+    _compute_D5,


These are internal functions (starting with an underscore) and should not be imported into the toponetx namespace.

ffl096 · 2023-06-27T08:26:01Z

test/utils/test_normalization.py

+    _compute_D1,
+    _compute_D2,
+    _compute_D3,
+    _compute_D5,


As far as I see these internal functions are not called directly here and should not be imported.

ffl096 · 2023-06-27T08:26:58Z

test/utils/test_normalization.py

+
+    L = csr_matrix(L).asfptype()
+    normalized_L = compute_laplacian_normalized_matrix(L)
+    expected_result = csr_matrix(


No need to construct a csr_matrix here when you convert it to dense arrays below again. Just use np.array.

ffl096 · 2023-06-27T08:27:11Z

test/utils/test_normalization.py

+    L = csr_matrix([[2.0, -1.0, 0.0], [-1.0, 3.0, -1.0], [0.0, -1.0, 2.0]])
+    Lx = csr_matrix([[1.0, 0.0, 0.0], [0.0, 0.0, 1.0], [0.0, 1.0, 0.0]])
+    normalized_Lx = compute_x_laplacian_normalized_matrix(L, Lx)
+    expected_result = csr_matrix([[0.25, 0.0, 0.0], [0.0, 0.0, 0.25], [0.0, 0.25, 0.0]])


ffl096 · 2023-06-27T08:27:21Z

test/utils/test_normalization.py

+    L = csr_matrix([[4.0, 0], [0.0, 4.0]])
+    Lx = csr_matrix([[0.0, 1.0], [1.0, 0.0]])
+    normalized_Lx = compute_x_laplacian_normalized_matrix(L, Lx)
+    expected_result = csr_matrix([[0.0, 0.25], [0.25, 0.0]])


ffl096 · 2023-06-27T08:30:35Z

toponetx/utils/normalization.py

+    ----------
+    A_opt : numpy.ndarray
+        The adjacency matrix.
+    add_identity : bool, optional


Suggested change

add_identity : bool, optional

add_identity : bool, default=False

In general optional boolean flags feel weird, there is nothing optional about them. They default to true or to false, that's it.

ffl096 · 2023-06-27T08:35:19Z

toponetx/utils/normalization.py

+    Notes
+    -----
+    This normalization is based on Kipf's formulation, which computes the row-sums,
+    constructs a diagonal matrix D^(-0.5), and applies the normalization as D^(-0.5) * A_opt * D^(-0.5).


Maybe format this a proper LaTeX formulas? This would render better on the documentation website. See https://numpydoc.readthedocs.io/en/latest/format.html#notes and their rendered example.

ffl096 · 2023-06-27T08:39:06Z

toponetx/utils/normalization.py

+    B : numpy.ndarray or scipy.sparse.csr_matrix
+        The asymmetric matrix.
+    is_sparse : bool, optional
+        If True, treat B as a sparse matrix.


I wonder whether that parameter is actually useful? Just test whether B is sparse (scipy.sparse.issparse) should be sufficient, right? Or is there ever a case where B is of sparse type but you want to set is_sparse=False?

ffl096 · 2023-06-27T08:40:39Z

toponetx/utils/normalization.py

+
+    Parameters
+    ----------
+    B1 : numpy array or scipy csr_matrix


numpy array and scipy csr_matrix should be replaced by np.ndarray and scipy.sparse.csr_matrix, respectively, throughout the document

ffl096 · 2023-06-27T08:41:56Z

toponetx/utils/normalization.py

+
+    Returns
+    -------
+    _ : numpy array or scipy csr_matrix


Please remove the _ : here and below. If a function returns just one value (as opposed to a tuple of values), just state the type.

ninamiolane · 2023-09-19T22:37:43Z

@mhajij @ffl096 update on this?

test/utils/test_normalization.py

toponetx/utils/normalization.py

devendragovil

Requesting some changes.

toponetx/utils/normalization.py

test/utils/test_normalization.py

devendragovil

References need to be reformatted.

devendragovil · 2023-11-17T21:26:30Z

toponetx/utils/normalization.py

+    [1] Schaub, M. T., Benson, A. R., Horn, P., Lippner, G., & Jadbabaie, A.
+        "Random walks on simplicial complexes and the normalized hodge 1-laplacian."
+
+    [2] Bunch, E., You, Q., Fung, G., & Singh, V.
+        "Simplicial 2-Complex Convolutional Neural Networks."
+    """


The references section is still not in the required format. See https://github.com/pyt-team/TopoModelX/blob/main/topomodelx/nn/simplicial/dist2cycle_layer.py for an example or here https://bashtage.github.io/sphinx-material/rst-cheatsheet/rst-cheatsheet.html

devendragovil

LGTM.

mhajij added 3 commits June 25, 2023 05:30

normalization added

7509f72

adding more unit tests to normalization

6c53d16

more unit testing

33501cd

adding norm to init

ee56ab5

mhajij requested a review from michaelschaub June 26, 2023 05:52

ffl096 requested changes Jun 26, 2023

View reviewed changes

toponetx/utils/normalization.py Show resolved Hide resolved

toponetx/utils/normalization.py Outdated Show resolved Hide resolved

toponetx/utils/normalization.py Outdated Show resolved Hide resolved

toponetx/utils/normalization.py Outdated Show resolved Hide resolved

michaelschaub reviewed Jun 26, 2023

View reviewed changes

toponetx/utils/normalization.py Outdated Show resolved Hide resolved

mhajij added 3 commits June 27, 2023 00:52

fixing naming

1ce0d38

fixing coo matrix

ebb8f94

simplify computing n laplacians

566f0a1

mhajij requested review from ffl096, michaelschaub and ninamiolane June 27, 2023 07:59

ffl096 requested changes Jun 27, 2023

View reviewed changes

ffl096 added the enhancement New feature or request label Jun 29, 2023

mhajij mentioned this pull request Nov 14, 2023

SCConv needs a full revision pyt-team/TopoModelX#253

Open

2 tasks

mhajij added 4 commits November 16, 2023 10:35

fixing docstrings and minor unit test changes

25fcf64

fix lint and unit tests

6cfbd34

adding numpy docstring fixes

219ee48

fixing unit test

361c910

mhajij requested review from ffl096 and devendragovil November 16, 2023 18:54

ffl096 requested changes Nov 17, 2023

View reviewed changes

mhajij added 3 commits November 17, 2023 08:11

fixing some docstrings

146ad76

small fix

e257932

fix lint errors

fea398f

USFCA-MSDS requested a review from ffl096 November 17, 2023 16:28

devendragovil requested changes Nov 17, 2023

View reviewed changes

toponetx/utils/normalization.py Outdated Show resolved Hide resolved

test/utils/test_normalization.py Outdated Show resolved Hide resolved

fix references numpydoc

362cc04

USFCA-MSDS requested a review from devendragovil November 17, 2023 19:26

devendragovil requested changes Nov 17, 2023

View reviewed changes

fixing refs

396f70f

mhajij requested a review from devendragovil November 17, 2023 21:59

devendragovil approved these changes Nov 17, 2023

View reviewed changes

mhajij closed this Nov 18, 2023

mhajij reopened this Nov 18, 2023

mhajij merged commit 3c7290f into main Nov 18, 2023
12 of 14 checks passed

ffl096 deleted the normalization branch February 2, 2024 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

normalization added #176

normalization added #176

mhajij commented Jun 25, 2023 •

edited

Loading

codecov bot commented Jun 26, 2023 •

edited

Loading

michaelschaub Jun 26, 2023

ninamiolane Jun 27, 2023

mhajij Jun 27, 2023

ffl096 Jun 27, 2023

ffl096 Jun 27, 2023

ffl096 Jun 27, 2023

ffl096 Jun 27, 2023

ffl096 Jun 27, 2023

ffl096 Jun 27, 2023

ffl096 Jun 27, 2023

ffl096 Jun 27, 2023 •

edited

Loading

ffl096 Jun 27, 2023

ffl096 Jun 27, 2023

ninamiolane commented Sep 19, 2023

devendragovil left a comment

devendragovil left a comment

devendragovil Nov 17, 2023

devendragovil left a comment

	add_identity : bool, optional
	add_identity : bool, default=False

normalization added #176

normalization added #176

Conversation

mhajij commented Jun 25, 2023 • edited Loading

codecov bot commented Jun 26, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffl096 Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninamiolane commented Sep 19, 2023

devendragovil left a comment

Choose a reason for hiding this comment

devendragovil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devendragovil left a comment

Choose a reason for hiding this comment

mhajij commented Jun 25, 2023 •

edited

Loading

codecov bot commented Jun 26, 2023 •

edited

Loading

ffl096 Jun 27, 2023 •

edited

Loading