PABLO: add node and edge periodic neighbours #58

edoardolombardi · 2020-03-17T10:45:32Z

It enables finding node and edge neighbours with periodic boundaries (#55).

andrea-iob · 2020-03-17T12:07:17Z

examples/CMakeLists.txt

@@ -62,7 +62,8 @@ if(BUILD_EXAMPLES)
 	list(APPEND EXAMPLE_LIST "PABLO_example_00007")
 	list(APPEND EXAMPLE_LIST "PABLO_example_00008")
 	list(APPEND EXAMPLE_LIST "PABLO_example_00009")
-	list(APPEND EXAMPLE_LIST "PABLO_example_00010")
+    list(APPEND EXAMPLE_LIST "PABLO_example_00010")
+    list(APPEND EXAMPLE_LIST "PABLO_example_00011")


Example 00011 is not introduced by this commit, its introduced in a2f6106. This commit only modifies the example.

I think that something went wrong with my edit/squash/fixup...

andrea-iob · 2020-03-17T12:10:03Z

src/PABLO/Octant.cpp

@@ -1544,7 +1544,7 @@ Octant Octant::computePeriodicOctant(uint8_t iface) const {
 	uint32_t dh = this->getLogicalSize();

 	if (!m_info[iface]){
-		return this->computeMorton();
+		return *this;
 	}


This seems a fix that should be on a stand alone commit.

andrea-iob · 2020-03-17T12:13:37Z

src/PABLO/LocalTree.hpp

 	void 		findGhostNeighbours(uint32_t idx, uint8_t iface, u32vector & neighbours, bvector & isghost) const;
 	void 		findGhostEdgeNeighbours(uint32_t idx, uint8_t iedge, u32vector & neighbours, bvector & isghost) const;
 	void 		findGhostNodeNeighbours(uint32_t idx, uint8_t inode, u32vector & neighbours, bvector & isghost) const;
-	BITPIT_DEPRECATED(void 		findGhostPeriodicNeighbours(const Octant* oct, uint8_t iface, u32vector & neighbours) const);
-


The commit title is "PABLO: added node and edge periodic neighbors", but you are also removing two functions. Better remove this two functions in a separate commit.

As above...

andrea-iob · 2020-03-17T12:15:23Z

src/PABLO/Octant.cpp

+    if (!m_info[iface1] && !m_info[iface2] && !m_info[iface3]){
+        return *this;
+    }
+    else{


Can 'getNodePeriodicCoord' be used to evaluate the coordinates instead of duplicating the code?

No, they deal with periodic octant in different way.

andrea-iob · 2020-03-17T12:15:39Z

src/PABLO/Octant.cpp

+    for (int idim=0; idim<m_dim; idim++){
+        cxyz[idim] = sm_treeConstants[m_dim].edgeCoeffs[iedge][idim];
+    }
+


Can 'getEdgePeriodicCoord' be used to evaluate the coordinates instead of duplicating the code?

No, they deal with periodic octant in different way.

andrea-iob · 2020-03-17T12:17:30Z

src/PABLO/LocalTree.cpp

                    u32array3 coordtry = m_ghosts[idxtry].getLogicalCoord();
-                    u32array3 coord1 = { {1,1,1} };
+                    array<int64_t,3> coord1 = { {1,1,1} };
                    u32array3 coordtry1 = { {1,1,1} };


Unrelated, please remove it.

No, it's correct.

andrea-iob · 2020-03-17T12:18:57Z

src/PABLO/LocalTree.cpp

            if (m_periodic[iface]){
                isperiodic = true;
-//                findPeriodicNeighbours(oct, iface, neighbours, isghost);
            }


These changes belong to the previous commit.

Yes, right.

andrea-iob · 2020-03-17T12:21:14Z

src/PABLO/Octant.hpp

+    std::array<int64_t,3> getPeriodicCoord(uint8_t iface) const;
+    std::array<int64_t,3> getNodePeriodicCoord(uint8_t inode) const;
+    std::array<int64_t,3> getEdgePeriodicCoord(uint8_t iedge) const;
+   	uint8_t getFamilySplittingNode() const;
 };


It seems that indentation is not coherent with the surrounding code.

andrea-iob · 2020-03-17T12:21:38Z

examples/PABLO_example_00011.cpp

+ *
+ *  bitpit
+ *
+ *  Copyright (C) 2015-2019 OPTIMAD engineering Srl


andrea-iob · 2020-03-17T14:36:00Z

src/PABLO/Octant.cpp

+        switch (iface1) {
+        case 0 :
+        {
+            if (m_info[0]){


There are constants to access the m_info entries (OctantInfo enum in Octant.hpp).

Ok, I'll use this enum.

andrea-iob · 2020-03-17T14:42:16Z

src/PABLO/Octant.cpp

+ */
+Octant Octant::computeNodePeriodicOctant(uint8_t inode) const {
+    Octant degOct(this->m_dim, this->m_level, this->m_x, this->m_y, this->m_z);
+    uint32_t maxLength = uint32_t(1)<<TreeConstants::MAX_LEVEL;


There is a constant for the max length: MAX_LENGTH in TreeConstants (should be accessible through sm_treeConstants).

Ah yes, I think I used some lines above....

andrea-iob · 2020-03-17T14:47:24Z

All Octant coordinates are uint32_t, whereas get*PeriodicCoord() functions return int64_t. Is this intentional?

andrea-iob · 2020-03-17T14:58:17Z

All Octant coordinates are uint32_t, whereas get*PeriodicCoord() functions return int64_t. Is this intentional?

Maybe I get it, is to allow negative numbers?

edoardolombardi · 2020-03-17T15:12:00Z

Yes, this is the reason (I quickly commented in the inline comment). Maybe there is some inconsistent compares in the code, I check form them.

andrea-iob · 2020-03-17T15:41:19Z

src/PABLO/Octant.cpp

+/*! Get the coordinates of the octant shifted through edge iedge and
+ * near the opposite periodic boundary (i.e. the coordinates considering this octant
+ * as a ghost periodic octant).
+ * The face through the octant is periodic has to be passed as boolean flags.


For this comments it seems there should be an additional argument that tells which are the periodic faces. (same applies for the other getPeriodicCoord() and computePeriodicOctant functions).

andrea-iob · 2020-03-17T15:46:12Z

I tried to check if the calculations in get*PeriodicCoord and compute*PeriodicOctant are correct, but I can't fully understand what these function do. If you want me to really check the calculations, I need some details on what is the purpose of the functions. However, if the functions pass the tests and I'm fine with them.

edoardolombardi · 2020-03-17T16:10:38Z

Sure:

compute periodic octant functions give an octant (used as virtual) of the same size of the current one placed over the periodic entity (face, edge or node) on the opposite side of the domain; e.g. over periodic boundaries 0/1 if the current octant is at the maximum x-coordinate of the domain, the returned periodic octant over the face (1) is placed at zero x-coordinate.
get periodic coordinates functions give the coordinates of the current octant considered as periodic ghost placed on the same relative size of the opposite periodic entity; i.e. considering the same current octant of the example above, the returned coordinates are the coordinate of the octant placed at x-coordinate equal to -h (where h is the size of the octant), on the same size of the periodic face on the opposite domain boundary (boundary 0). This octant is placed outside the logical domain, for this reason negative coordinates are needed.

edoardolombardi · 2020-03-18T14:37:19Z

PR updated.

…urs function Special function for finding periodic neighbours are no more needed and thus they are now dropped.

andrea-iob · 2020-03-25T13:07:47Z

I fixed the indentation in a couple of places and remove some "unused variables" warnings. I will do the marge later this afternoon.

edoardolombardi added the enhancement label Mar 17, 2020

edoardolombardi requested review from marcocisternino and andrea-iob March 17, 2020 10:45

andrea-iob reviewed Mar 17, 2020

View reviewed changes

examples/PABLO_example_00011.cpp Outdated

*

* bitpit

*

* Copyright (C) 2015-2019 OPTIMAD engineering Srl

Copy link

Member

andrea-iob Mar 17, 2020

Choose a reason for hiding this comment

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

andrea-iob reviewed Mar 17, 2020

View reviewed changes

edoardolombardi force-pushed the PABLO.add.node.and.edge.periodic.neighbours branch from 5138494 to e28da13 Compare March 17, 2020 17:52

edoardolombardi force-pushed the PABLO.add.node.and.edge.periodic.neighbours branch 3 times, most recently from 98fe864 to 2a8bf2c Compare March 23, 2020 11:53

marcocisternino approved these changes Mar 23, 2020

View reviewed changes

edoardolombardi added 6 commits March 25, 2020 14:05

PABLO: fix compute periodic octant

14d61f4

PABLO: add periodic neighbours to the output of standard find neighbo…

e508dc6

…urs function Special function for finding periodic neighbours are no more needed and thus they are now dropped.

PABLO: add support for finding node periodic neighbors

2c13210

PABLO: add support for finding edge periodic neighbors

cddafc6

examples/PABLO: add an example on finding neighbours in periodic trees

9934f09

PABLO: fix indentation of Octant.hpp

d0869b8

andrea-iob force-pushed the PABLO.add.node.and.edge.periodic.neighbours branch from 2a8bf2c to d0869b8 Compare March 25, 2020 13:06

andrea-iob self-requested a review March 25, 2020 13:08

andrea-iob approved these changes Mar 25, 2020

View reviewed changes

andrea-iob merged commit d0869b8 into master Mar 26, 2020

andrea-iob deleted the PABLO.add.node.and.edge.periodic.neighbours branch March 26, 2020 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PABLO: add node and edge periodic neighbours #58

PABLO: add node and edge periodic neighbours #58

edoardolombardi commented Mar 17, 2020

andrea-iob Mar 17, 2020

edoardolombardi Mar 17, 2020

andrea-iob Mar 17, 2020

andrea-iob Mar 17, 2020

edoardolombardi Mar 17, 2020

andrea-iob Mar 17, 2020

edoardolombardi Mar 17, 2020

andrea-iob Mar 17, 2020

edoardolombardi Mar 17, 2020

andrea-iob Mar 17, 2020

edoardolombardi Mar 17, 2020

andrea-iob Mar 17, 2020

edoardolombardi Mar 17, 2020

andrea-iob Mar 17, 2020

andrea-iob Mar 17, 2020

andrea-iob Mar 17, 2020

edoardolombardi Mar 17, 2020

andrea-iob Mar 17, 2020

edoardolombardi Mar 17, 2020

andrea-iob commented Mar 17, 2020

andrea-iob commented Mar 17, 2020

edoardolombardi commented Mar 17, 2020

andrea-iob Mar 17, 2020

andrea-iob commented Mar 17, 2020 •

edited

Loading

edoardolombardi commented Mar 17, 2020

edoardolombardi commented Mar 18, 2020 •

edited

Loading

andrea-iob commented Mar 25, 2020

PABLO: add node and edge periodic neighbours #58

PABLO: add node and edge periodic neighbours #58

Conversation

edoardolombardi commented Mar 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrea-iob commented Mar 17, 2020

andrea-iob commented Mar 17, 2020

edoardolombardi commented Mar 17, 2020

Choose a reason for hiding this comment

andrea-iob commented Mar 17, 2020 • edited Loading

edoardolombardi commented Mar 17, 2020

edoardolombardi commented Mar 18, 2020 • edited Loading

andrea-iob commented Mar 25, 2020

andrea-iob commented Mar 17, 2020 •

edited

Loading

edoardolombardi commented Mar 18, 2020 •

edited

Loading