[BREAKING CHANGES] Clean up adjacency and related methods

[josefhoppe]

As we discussed, I moved _incidence_to_adjacency to the utility package. I also cleaned up duplicate adjacency methods in CellComplex, resulting in breaking changes.

• I don't think removing s breaks anything as it didn't work before
• However, removing node_ajacency_matrix and cell_adjacency_matrix will inevitably break something (they could be re-added as proxy methods for adjacency_matrix if you prefer)
• Also, we could think about changing the behavior of adjacency_matrix: Currently, the default is to use the lower adjacency. For rank 0, this does not really make sense. I'm not sure what I prefer:
  • changing it to default to upper for rank 0, but lower for other ranks is the way it is used most often, but then the default is not consistent over all ranks
  • changing it to always default to upper or lower would be consistent, but then it would need to be specified more often.

[codecov]

Codecov Report

Merging #94 (4e16bbf) into main (71e840e) will decrease coverage by 0.19%.
The diff coverage is 64.15%.

❗ Current head 4e16bbf differs from pull request most recent head 93316cd. Consider uploading reports for the commit 93316cd to get more accurate results

@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
- Coverage   61.62%   61.44%   -0.19%     
==========================================
  Files          22       22              
  Lines        2541     2534       -7     
==========================================
- Hits         1566     1557       -9     
- Misses        975      977       +2     

Impacted Files	Coverage Δ
toponetx/classes/cell_complex.py	63.86% <48.00%> (-0.41%)	⬇️
toponetx/classes/combinatorial_complex.py	50.83% <53.84%> (-1.42%)	⬇️
toponetx/algorithms/spectrum.py	93.42% <100.00%> (+0.08%)	⬆️
toponetx/utils/structure.py	97.05% <100.00%> (+1.22%)	⬆️

... and 1 file with indirect coverage changes

[ffl096]

I would argue that two separate functions (e.g., lower_adjacency_matrix and upper_adjacency_matrix) makes for a cleaner API, as it is more discoverable than a boolean flag and easier to understand from the consumer's side.

Additionally, that would resolve the issue with the default value, as the consumer is forced to actively choose between both.

[ffl096]

That being said, isn't that what adjacency_matrix and coadjacency_matrix are for, anyway?

[josefhoppe]

@ffl096 I think you're right, thanks. I'll update the code accordingly.

[ninamiolane]

Thanks! A few questions @josefhoppe

• Why: "As we discussed, I moved _incidence_to_adjacency to the utility package."?
• I think using the lower adjacency as a default is intuitive, because "adjacency" in graphs means lower adjacency.

I'm puzzled with flake8-docstrings is not picking up on the docstring formatting: docstrings should start with verb in imperative mode, which is not respected in one of your test files.

[ninamiolane]

Fix Docstrings formatting:

• Summary should fit on one line directly after """.
• Summary should start with a verb in infinitve mode --> Test

[ninamiolane]

This docstrings does not follow the signature of the function.

[josefhoppe]

Why: "As we discussed, I moved _incidence_to_adjacency to the utility package."?

Maybe I misunderstood you. I included it in #40 as it existed in mostly the same form in both CombinatorialComplex and CellComplex and you said you generally agreed with de-duplicating code (now that I think of it, that could also be a reference to other de-duplications).

Either way, I still think that it makes sense to move it to the utility package, but if you disagree we can also revert that change.

I think using the lower adjacency as a default is intuitive, because "adjacency" in graphs means lower adjacency.

Does it? Nodes (0-cells) are upper adjacent to each other via edges (1-cells), aren't they?

Regarding the doc strings: Thanks for catching that, I'll fix the ones you pointed out.

[mhajij]

the duplication issue between many complexes is indeed a problem. NetworkX does not attach the adjacency matrix as a graph method but rather as a part of the linear algebra module :
https://github.com/networkx/networkx/blob/main/networkx/linalg/graphmatrix.py

same for the incidence matrix. I am not sure we can do that for all complexes though since the parameters might differ (CC for instance takes two arguments).

[mhajij]

lower_adjacency_matrix and upper_adjacency_matrix are the same thing as adjacency matrix and coadjacency matrix. Reason why I kept adjacency and coadjacency matrix is to be consist with graphs when the rank is zero. I am not sure if our output convention matches that of networkx's adjacnecy matrix but preferably so.