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

Clean up and simplify CellComplex #40

Closed
josefhoppe opened this issue May 19, 2023 · 4 comments
Closed

Clean up and simplify CellComplex #40

josefhoppe opened this issue May 19, 2023 · 4 comments
Assignees

Comments

@josefhoppe
Copy link
Collaborator

The CellComplex class contains some non-working, unused, or duplicate code / methods. This should be cleaned up to make it easier for both development and use.

@josefhoppe josefhoppe self-assigned this May 19, 2023
@josefhoppe
Copy link
Collaborator Author

josefhoppe commented May 22, 2023

After looking into it, I'm not sure what is useful and what isn't.

It feels a bit like a lot of the code was copied from CombinatorialComplex: For example, graph connectivity has a parameter s for how connected cells need to be

  • This also breaks a lot of stuff since the s parameter is passed to functions that don't support it
  • This also brings different implementations of adjacency matrix (one general, three seperate for node, edge, cell)
  • CellComplex._incidence_to_adjacency() is copied 1:1 from CombinatorialComplex except for s. Maybe generalize and move it to a utility package?

I also have some minor remarks where I'm not sure what the best way to proceed is:

  • CellComplex.incidence_matrix() for nodes returns a $1\times n$ matrix instead of $0\times n$ (seems incorrect, but maybe something depends on this behavior?)
    • with index=True, it returns an empty row index list, but matrix has one row.
    • with index=True it also returns the indices in the wrong (inconsistent with higher ranks) order
  • Interesting Property of the CellComplex Laplacian (may be correct, but unexpected): If two cells (1,2,3,4) and (1,2,4,3) exist, (1,2) and (3,4) induce the same orientation on one cell and opposite orientations on the other. This means the laplacian will have an entry of $0 (=-1 + 1)$ for these two edges.
    • In CellComplex, this also affects the adjacency_matrix as it takes the laplacian instead of using the square of the absolute of the incidence matrix. (would the correct adjacency then be 2 for twice adjacent or 1 for 'yes, adjacent'?)
  • k_hop_coincidence_matrix: doesn't work as coincidence_matrix doesn't exist. (also, I'm not sure why we have it. It's just the coincidence of the k-hop (lower and upper) adjacency, isn't it? the last step seems trivial enough to leave it to the user unless the overall concept is very important)
  • get_cell_attributes: default rank None, doesn't return anything and no error for rank None -> Easy to accidentally use incorrectly, like in get_filtration (see my bugfix in 0d096a4). I suggest to remove the default value.

Finally, I found some things that are straightforward bugs (I just listed them so I won't forget about it):

  • CellComplex.restrict_to_cells() discards attributes

@josefhoppe
Copy link
Collaborator Author

CellComplex.k_hop_coincidence_matrix is currently not working since CellComplex.coincidence_matrix does not exist. Since I don't understand its purpose: Do we need it?

It seems relatively trivial to compute for the user, so unless it is frequently needed for something I'd suggest we remove it (along with CellComplex.k_hop_incidence_matrix; but that one currently works)

@mhajij @ninamiolane

@mhajij
Copy link
Member

mhajij commented May 25, 2023

I agree with you. Let’s remove it.

@mhajij
Copy link
Member

mhajij commented May 25, 2023

I’ll discuss the bugs here later today in details.

michaelschaub added a commit that referenced this issue May 25, 2023
Fix CellComplex.restrict_to_cells (see #40)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants