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

Normalize edge "order" and "size" #67

Closed
leotrs opened this issue Mar 23, 2022 · 9 comments
Closed

Normalize edge "order" and "size" #67

leotrs opened this issue Mar 23, 2022 · 9 comments

Comments

@leotrs
Copy link
Collaborator

leotrs commented Mar 23, 2022

The words "order" and "size" are used throughout the codebase to refer to the number of nodes in an edge (where order = size-1). I'd suggest we choose one and remove all instances of the other. For example, we have H.edge_size but H.edges_of_order.

@leotrs
Copy link
Collaborator Author

leotrs commented Mar 23, 2022

FWIW, I'd rather keep order so that we don't have problems when implementing simplicial complexes.

@iaciac
Copy link
Collaborator

iaciac commented Mar 23, 2022

I'm more than OK with order!

@maximelucas
Copy link
Collaborator

Good point. I'm fine with order too.

@nwlandry
Copy link
Collaborator

nwlandry commented Apr 9, 2022

I'm okay with doing this, but will admit that this will be a bit of habit for me to break :). Along these lines, is the biggest remaining change the IDDegreeView class?

@nwlandry
Copy link
Collaborator

nwlandry commented Apr 9, 2022

One thing that probably will need to change is that we may not be able to effectively use IDDegreeView class as a base class if we choose to use edge_order instead of edge_size, because degree and edge order will be off by one from each other.

@leotrs
Copy link
Collaborator Author

leotrs commented Apr 11, 2022

Oh jeez that complicates things.

@leotrs
Copy link
Collaborator Author

leotrs commented Apr 20, 2022

Related to #82

@nwlandry
Copy link
Collaborator

nwlandry commented Jul 4, 2022

This was fixed with EdgeStats, no?

@leotrs
Copy link
Collaborator Author

leotrs commented Jul 4, 2022

Kind of. We now have both order and size, so any EdgeStat (or any other part of the codebase that is aware of the stats package) can choose which one to use. So on that front we are fine.

What has not been addressed is the fact that currently, SimplicialComplex inherits from Hypergraph and that is Very Messy And Bad.

I guess this particular issue is about the first problem, not the latter, so let's close this. For more discussion of the latter, continue on #82 .

@leotrs leotrs closed this as completed Jul 4, 2022
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

4 participants