Skip to content

Hexagonal grid graph#2262

Merged
ntamas merged 6 commits into
igraph:masterfrom
rfulekjames:hexagonal-grid-graph
Dec 1, 2022
Merged

Hexagonal grid graph#2262
ntamas merged 6 commits into
igraph:masterfrom
rfulekjames:hexagonal-grid-graph

Conversation

@rfulekjames
Copy link
Copy Markdown
Contributor

@rfulekjames rfulekjames commented Nov 25, 2022

Implements a hexagonal grid #1843 dual to the triangular grid
Examples for some inputs from the unit test:
for dimvector=(5), directed=true, mutual=false

output

for dimvector=(4, 5), directed=true, mutual=true
output

for dimvector=(3, 4, 5), directed=false
output

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 25, 2022

Codecov Report

Merging #2262 (2905e69) into master (9295a00) will increase coverage by 0.02%.
The diff coverage is 98.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2262      +/-   ##
==========================================
+ Coverage   83.30%   83.33%   +0.02%     
==========================================
  Files         371      371              
  Lines       60910    61018     +108     
==========================================
+ Hits        50739    50847     +108     
  Misses      10171    10171              
Impacted Files Coverage Δ
src/constructors/lattices.c 97.63% <98.23%> (+2.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9295a00...2905e69. Read the comment docs.

@szhorvat szhorvat marked this pull request as ready for review November 25, 2022 11:36
@szhorvat
Copy link
Copy Markdown
Member

szhorvat commented Nov 25, 2022

Thanks for working on this, this will be great!

I have not looked at either the code or its output yet, just wanted to make sure we're on the same page about the idea.

The input should be the same as for the triangular lattice, and the output should be a hex lattice whose dual is the triangular lattice obtained with the same parameters. Here are some illustrations:

image

image

image

Notice that every red tri lattice node is in the centre of a blue hexagon, but the reverse is not true. Not every blue node is in the middle of a triangle. My reasoning for this choice: if we put a node in the centre of every triangle, we will not always get complete hexagons. There will be some degree-1 nodes at the corners.

I do not have hex lattice graphs implemented in Mathematica, so this implementation doesn't have to match anything. If you are wondering how I generated these: I have an implementation of various geometric meshes: not quite the same thing as a graph, but one can derive a graph from it, and in case of the hexagonal mesh it happens to give nice results.

It would be great to index the vertices in a way that it wouldn't be too hard to generate coordinates for them to get nice plots like above. The way you implemented the triangular lattice makes this easy, so that's great.

If you feel that in order to allow the tri and hex lattices to match, you should change something in the tri lattice implementation, feel free to do that.

Let us know if you have any questions! When the PR is ready, mark it as "ready for review".

@szhorvat szhorvat added this to the 0.10.3 milestone Nov 25, 2022
@rfulekjames
Copy link
Copy Markdown
Contributor Author

I do not have hex lattice graphs implemented in Mathematica, so this implementation doesn't have to match anything. If you are wondering how I generated these: I have an implementation of various geometric meshes: not quite the same thing as a graph, but one can derive a graph from it, and in case of the hexagonal mesh it happens to give nice results.

@szhorvat Thanks for the link! For me it is also easy to draw it with python-igraph using kamada_kawai layout. Just added the figures in my comment from the above.

The labelling at this point is row based similarly as in the case of the triangular grid if interpretting the lattice as a brick wall, which should allow for constructing a layout but I expect it to be a bit more intricate than in the case of the triangular lattice.

@szhorvat szhorvat marked this pull request as draft November 25, 2022 15:34
@rfulekjames rfulekjames marked this pull request as ready for review November 25, 2022 15:34
@szhorvat
Copy link
Copy Markdown
Member

It seems I accidentally marked this as "ready for review". I reverted to draft, just let us know when you consider it ready.

The plots look great!

@rfulekjames
Copy link
Copy Markdown
Contributor Author

I marked it ready for review :-) ! @szhorvat

Copy link
Copy Markdown
Member

@szhorvat szhorvat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some doc formatting for now.

In the switch statements, can you un-indent the body by one level so that the case lines up with the switch?

The screenshots you shared show that this works well, so I don't expect issues. I'll do partial integration into the Mma interface and test it a bit, then I think we can merge.

Comment thread src/constructors/lattices.c Outdated
Comment thread src/constructors/lattices.c Outdated
Comment thread src/constructors/lattices.c Outdated
Comment thread src/constructors/lattices.c
Comment thread src/constructors/lattices.c Outdated

IGRAPH_VECTOR_INT_INIT_FINALLY(&edges, 0);

COMPUTE_NUMBER_OF_VERTICES
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to embed opening and closing braces inside the definition of COMPUTE_NUMBER_OF_VERTICES? This would make it possible to add a semicolon after COMPUTE_NUMBEF_OF_VERTICES when it's being used here; not having a semicolon at the end of the line might confuse syntax highlighters that are not advanced enough to recognize that we are working with a macro here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of just using { ... }, please use do { .... } while (0) when defining the macro. This will shut up some linters that would complain about an empty statement when the semicolon is added.

Actually, I'm wondering if this is better converted a real function ... but that's not really important.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if it is Ok now.

@szhorvat
Copy link
Copy Markdown
Member

I won't have time to integrate this into the Mma interface for a while. Feel free to merge, @ntamas, once you're happy.

@rfulekjames rfulekjames requested a review from szhorvat December 1, 2022 02:46
@rfulekjames rfulekjames requested review from ntamas and removed request for szhorvat December 1, 2022 03:01
@ntamas ntamas merged commit 88001e6 into igraph:master Dec 1, 2022
@ntamas
Copy link
Copy Markdown
Member

ntamas commented Dec 1, 2022

Thanks a lot!

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

Successfully merging this pull request may close these issues.

3 participants