pyt-team
A simplicial complex has dimension equal to the size of its maximal s…
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.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
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.
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 : : https://github.com/pyt-team/TopoNetX/blob/7b8fa8b4ff56571a53999823b97ca78e20c7e86c/toponetx/classes/cell_complex.py#L229
whereas rank identifies the "dimension" of a particular skeleton in the larger complex. In other words : 0=< rank<= max_dim
@USFCA-MSDS Therefore, this means that we can merge this pull request right? @ffl096
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.
@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.
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.
@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?
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.
