alexbovet / flow_stability

Flow stability framework for dynamic community detection in temporal networks
GNU Lesser General Public License v3.0
24 stars 4 forks source link

Sparse autocov matrix classes #48

Open j-i-l opened 1 week ago

j-i-l commented 1 week ago

Currently we have two of them:

https://github.com/alexbovet/flow_stability/blob/8648a8bd715e3b086ed13cb7453d41754c176398/src/flowstab/SparseStochMat.py#L1175-L1176

and

https://github.com/alexbovet/flow_stability/blob/8648a8bd715e3b086ed13cb7453d41754c176398/src/flowstab/SparseStochMat.py#L1454-L1455

With a much larger usage pattern for sparse_autocov_mat. Actually, sparse_autocov_csr_mat only occurs twice as potential substitute for `sparse_autocov_mat' here and here thus the called methods overlap.

I did not check every method in detail, but to me it seems that sparse_autocov_csr_mat could either be dropped completely, or inherit from sparse_autocov_mat and overwrite some key methods.

@alexbovet what is your thought on this?

j-i-l commented 1 week ago

Not really an issue as with #44 we are going to remove the non-cython implementations, however:


https://github.com/alexbovet/flow_stability/blob/8648a8bd715e3b086ed13cb7453d41754c176398/src/flowstab/SparseStochMat.py#L1866

uses non-existing attribute _S in the non-cython case:

https://github.com/alexbovet/flow_stability/blob/8648a8bd715e3b086ed13cb7453d41754c176398/src/flowstab/SparseStochMat.py#L1898-L1901

same is true for the non-cython condition in

https://github.com/alexbovet/flow_stability/blob/8648a8bd715e3b086ed13cb7453d41754c176398/src/flowstab/SparseStochMat.py#L1830

j-i-l commented 1 week ago

See also https://github.com/alexbovet/flow_stability/pull/35#issuecomment-2355520641

alexbovet commented 2 days ago

I did not check every method in detail, but to me it seems that sparse_autocov_csr_mat could either be dropped completely, or inherit from sparse_autocov_mat and overwrite some key methods.

@alexbovet what is your thought on this?

Yes, we can drop sparse_autocov_csr_mat.