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

A simplicial complex has dimension equal to the size of its maximal s… #336

Merged
merged 6 commits into from
Jul 8, 2024

Conversation

rballeba
Copy link
Contributor

@rballeba rballeba commented Feb 6, 2024

A simplicial complex has dimension equal to the size of its maximal set minus one. If, for example, a simplicial complex has dimension one, then, it has simplices of at most two vertices, with at least one having exactly two vertices. Currently, the method graph_to_clique_complex was considering that the dimension of a simplex is exactly the number of vertices in it. This commit solves this problem. Also, it only copies the edge attributes from the networkx graph if the resulting simplicial complex has dimension greater or equal than one.

…et minus one. If, for example, a simplicial complex has dimension one, then, it has simplices of at most two vertices, with at least one having exactly two vertices. Currently, the method was graph_to_clique_complex was considering that the dimension of a simplex is exactly the number of vertices in it. This commit solves this problem. Also, it only copies the edge attributes from the networkx graph if the resulting simplicial complex has dimension greater or equal than one.
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.40%. Comparing base (ced2e43) to head (000d4e0).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #336      +/-   ##
==========================================
+ Coverage   97.36%   97.40%   +0.04%     
==========================================
  Files          38       38              
  Lines        3561     3545      -16     
==========================================
- Hits         3467     3453      -14     
+ Misses         94       92       -2     

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

@ffl096
Copy link
Member

ffl096 commented Feb 6, 2024

I'm on the fence on this one. The dimension that is meant here is the sizes of the cliques, not of the simplicial complex. We call the later one rank throughout the code base.

The direction to go would be to either

  • rename from dimension to rank and apply the suggested changes, or
  • leave the function as-is and maybe make it clearer that the parameter is concerned with clique sizes.

@rballeba
Copy link
Contributor Author

rballeba commented Feb 6, 2024

As I user, I would expect max_dim to correspond to the dim of the object I am generating. Is there any other use of the term dimension for graphs that coincides with the length and not the length minus one in the literature? @mhajij

By the way, the pull request is also solving a problem when the user is requesting to generate a point cloud. It is not a usual thing, but it can happen.

@ffl096 ffl096 added bug Something isn't working discussion labels Feb 6, 2024
@USFCA-MSDS
Copy link
Contributor

USFCA-MSDS commented Feb 9, 2024

As I user, I would expect max_dim to correspond to the dim of the object I am generating. Is there any other use of the term dimension for graphs that coincides with the length and not the length minus one in the literature? @mhajij

By the way, the pull request is also solving a problem when the user is requesting to generate a point cloud. It is not a usual thing, but it can happen.

@rballeba @ffl096
We are using both terms : dimension means the dimension of the of the complex, for instance in CellComplex : :

def dim(self) -> int:

whereas rank identifies the "dimension" of a particular skeleton in the larger complex. In other words : 0=< rank<= max_dim

@rballeba
Copy link
Contributor Author

@USFCA-MSDS Therefore, this means that we can merge this pull request right? @ffl096

@ffl096
Copy link
Member

ffl096 commented Feb 19, 2024

My point above still stands: If we want to change the behaviour, the parameter must be renamed. As of right now, this is a hidden break of any consumer code (existing code will run without any error but with changed result!).

The docstring should be updated as well, it still describes the current behaviour.

@rballeba
Copy link
Contributor Author

@ffl096 I see this in the docstring:

 max_dim : int, optional
        The max dimension of the cliques in the output clique complex.

This is what was wrong before. Could you point me what should I change from the docstring?

Regarding changing the code. I'm not a usual maintainer, so I would prefer to avoid larger changes in the codebase. If you think that this pull request does not fulfill the needs of the project, we can discard it. Either way, if we discard this modification, we should remark that max_dim is no the maximum dimension of the simplicial complex.

@ffl096
Copy link
Member

ffl096 commented Feb 19, 2024

This is what was wrong before. Could you point me what should I change from the docstring?

It was not. It specifically talks about the dimensions of cliques. I see that this may be confusing and hence I am not opposed to this change. However, if the function should be about the simplex rank, this has to be stated there.

Regarding changing the code. I'm not a usual maintainer, so I would prefer to avoid larger changes in the codebase.

Not sure what you mean by larger changes to the codebase? Its just about renaming the parameter.

@rballeba
Copy link
Contributor Author

rballeba commented Feb 26, 2024

@ffl096 I have not found any standard definition of dimension of a clique. Could you point to any reference where this term is used explicitly? If we see the clique as a simplex, then it has dimension equal to the number of vertices minus one. What I have found is the size of a clique, defined as its number of vertices https://mathworld.wolfram.com/Clique.html

About renaming the parameter, you mean renaming the parameter of the function or all the parameters related to the dimension of cliques?

@ffl096
Copy link
Member

ffl096 commented Mar 5, 2024

About renaming the parameter, you mean renaming the parameter of the function or all the parameters related to the dimension of cliques?

Rename max_dim to max_rank in this function.

@mhajij
Copy link
Member

mhajij commented Apr 4, 2024

@rballeba @ffl096 what is the status of this PR?

@ffl096 ffl096 added this to the 0.1.0 milestone Jun 27, 2024
@ffl096 ffl096 merged commit a78cd6a into pyt-team:main Jul 8, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants