Compact Triangulation Integration #674

guoxiliu · 2021-10-25T22:06:37Z

Thanks for contributing to TTK!

Before submitting your pull request, please:

Review our Contributor Guidelines, in particular regarding code formatting (with clang-format) and continuous integration.
Please provide a quick description of your contributions below:
This PR adds a compact triangulation data structure and also a preconditioning plugin for it.

Add the global cache size variable

julien-tierny · 2021-10-27T11:02:20Z

thanks a lot for your PR.

it seems that the CI identified a couple of issues with this PR.
these need to be solved before we move on with the code review.

could you please make sure:

to clang-format your code properly (see https://github.com/topology-tool-kit/ttk/blob/dev/CONTRIBUTING.md)
that your code also compiles fine with the following configuration: KAMIKAZE=FALSE, OPENMP=FALSE, 64BIT_IDS=FALSE, Release (the CI failed to build there, see the error messages there: https://github.com/topology-tool-kit/ttk/runs/4017913908?check_suite_focus=true#logs)

Thanks!

guoxiliu · 2021-10-27T12:47:08Z

Thanks, @julien-tierny. I will continue to work on these issues.

hi @guoxiliu

thanks a lot for your PR.

it seems that the CI identified a couple of issues with this PR. these need to be solved before we move on with the code review.

could you please make sure:

to clang-format your code properly (see https://github.com/topology-tool-kit/ttk/blob/dev/CONTRIBUTING.md)

that your code also compiles fine with the following configuration: KAMIKAZE=FALSE, OPENMP=FALSE, 64BIT_IDS=FALSE, Release (the CI failed to build there, see the error messages there: https://github.com/topology-tool-kit/ttk/runs/4017913908?check_suite_focus=true#logs)

Thanks!

julien-tierny

I have started a review. It is not finished at the moment.
I will resume it asap.

core/base/compactTriangulation/CompactTriangulation.cpp

core/base/compactTriangulation/CompactTriangulation.h

Update the reference in vtk header and xml file

julien-tierny

hi @guoxiliu

sorry for the late reply but here's my review for this PR.
if you could fix the few remarks I added in there, that would be wonderful.
maybe @pierre-guillou, you could also review the PR if you have time.

once the above remarks are addressed, I'll go ahead and run experiments with the code.

thanks!

core/base/compactTriangulation/CompactTriangulation.h

core/vtk/ttkCompactTriangulationPreconditioning/ttkCompactTriangulationPreconditioning.cpp

core/base/compactTriangulationPreconditioning/Octree.h

core/vtk/ttkCompactTriangulationPreconditioning/ttkCompactTriangulationPreconditioning.cpp

Move Octree to the base folder Remove debug message prefix in vtk file

julien-tierny

hi @guoxiliu
thanks a lot for your quick fix.
I have found yet a few occurrences of dynamic allocation.
could you also address these and let me know when you're done?
thanks!

core/base/octree/Octree.cpp

core/base/octree/Octree.h

julien-tierny · 2021-11-21T07:10:31Z

ok, thanks @guoxiliu
that looks all good to me.
I'll start to play around and experiment with the code a little bit.
@pierre-guillou could you have a look at this PR too?
thanks

julien-tierny · 2021-11-21T07:40:31Z

as a side note, I switched the CompactTriangulationPreconditioning to operate on vtkPointSet inputs (and outputs) instead of the more general vtkDataSet.
specifically, there is no need to call it on a regular grid for instance (and it actually crashed there).

paraview/xmls/CompactTriangulationPreconditioning.xml

pierre-guillou

Hi @guoxiliu!
I've looked at this PR and written a few comments on minor points. I hope I'm not too picky...

pierre-guillou · 2021-11-22T09:39:53Z

core/base/octree/Octree.h

+using namespace ttk;
+using namespace std;


I'm a bit concerned with these using namespace directives inside a header file since, through preprocessor inclusion, this can have non-local effects on other parts of the TTK codebase. Would it be possible to remove those and use instead fully qualified types (with ttk:: and std:: prefixes)? (using namespace in .cpp source files is fine though)

pierre-guillou · 2021-11-22T09:44:25Z

core/base/octree/Octree.h

+class OctreeNode {
+public:
+  OctreeNode() {
+    OctreeNode(0);


This creates a temporary object that is never used. Did you intend OctreeNode() : OctreeNode(0) {} instead? It is flagged by clang-tidy in the check_code CI workflow, first lint_code job, Use clang-tidy to lint code step as bugprone-undelegated-constructor

pierre-guillou · 2021-11-22T09:46:09Z

core/base/octree/Octree.cpp

+using namespace ttk;
+
+Octree::Octree(const AbstractTriangulation *t) {
+  Octree(t, 1000);


This creates a temporary object. Did you intend to call Octree::Octree(const AbstractTriangulation *t):Octree(t, 1000) {} instead?

pierre-guillou · 2021-11-22T09:54:18Z

core/base/octree/Octree.cpp

+  uint32_t location;
+  float ncenter[3] = {0.0}, nsize[3] = {0.0};
+  for(int i = 0; i < dim; i++) {
+    SimplexId vertexId;


Due to the internals of getCellVertex, this variable needs to be initialized (otherwise, a garbage value might be propagated). A simple SimplexId vertexId{}; with two braces should suffice to zero-initialize the variable. This is flagged by the clang static analyzer (check_code CI workflow, second lint_code job, Use Clang static analyzer step).

pierre-guillou · 2021-11-22T09:58:58Z

core/base/octree/Octree.h

+   */
+  uint32_t
+    getChildLocation(uint32_t parLoc, SimplexId vertexId, float centerArr[3]) {
+    float xval, yval, zval;


Those values should also be zero-initialized: float xval{}, yval{}, zval{};

pierre-guillou · 2021-11-22T10:13:50Z

core/base/compactTriangulation/CompactTriangulation.h

+    /**
+     * Reorder the input cells.
+     */
+    int reorderCells(const std::vector<SimplexId> &vertexMap,


inline or move to .cpp?

pierre-guillou · 2021-11-22T10:17:58Z

core/vtk/ttkCompactTriangulationPreconditioning/ttkCompactTriangulationPreconditioning.h

+#include <vtkCharArray.h>
+#include <vtkDataArray.h>
+#include <vtkDataSet.h>
+#include <vtkDataSetAlgorithm.h>
+#include <vtkDoubleArray.h>
+#include <vtkFiltersCoreModule.h>
+#include <vtkFloatArray.h>
+#include <vtkInformation.h>
+#include <vtkIntArray.h>
+#include <vtkObjectFactory.h>
+#include <vtkPointData.h>
+#include <vtkSmartPointer.h>
+#include <vtkUnstructuredGrid.h>


A lot of those includes can be moved to the .cpp

pierre-guillou · 2021-11-22T10:27:30Z

core/base/octree/Octree.h

+  float center[3];
+  float size[3];


Maybe use std::array<float, 3> instead of C-array? std::array are zero-initialized and have additional methods (begin(), end(), ...) that make them compatible with std::vector. To get a C-array like pointer, use the data() method.

pierre-guillou · 2021-11-22T10:44:24Z

core/vtk/ttkCompactTriangulationPreconditioning/ttkCompactTriangulationPreconditioning.cpp

+  std::vector<std::string>::iterator iter = scalarFields.begin();
+  while(iter != scalarFields.end()) {


A range-based for loop (for(const auto &s : scalarFields)) should be more readable here

pierre-guillou · 2021-11-22T10:44:54Z

core/vtk/ttkCompactTriangulationPreconditioning/ttkCompactTriangulationPreconditioning.cpp

+  iter = scalarFields.begin();
+  while(iter != scalarFields.end()) {


A range-based for loop might be preferred here too

julien-tierny · 2021-12-02T06:54:16Z

@pierre-guillou
could you please double-check @guoxiliu 's latest commits and let me know if you approve all edits?
if so, I guess we'd be good to go for the merge :)

pierre-guillou · 2021-12-03T12:02:41Z

Hi @guoxiliu,

Thanks for taking my remarks into account.
I looked at your last changes and played a bit with the code. I have some last suggestions in the form of the following diffs (you can apply them by copying these files in the root of your TTK repo then using git am on them - or reproducing the changes by hand):

0001-Reduce-variables-scope.txt
0002-Clear-state-before-computation.txt
0003-Call-parent-class-copy-constructor.txt

Also, I noticed several times some weird memory issue that filled the 64GB of RAM of my computer and makes it swap. To trigger them, I load the noisyTerrain.vtu dataset in ParaView, apply CompactTriangulationPreconditionning then launching PersistenceDiagram. Sometimes, this is enough to trigger the phenomenon, otherwise, fiddling with the input scalar fields in the CompactTriangulationPreconditionning filter causes it (my 2nd patch might also have an effect there). Using heaptrack, I found that the faulty memory allocations occured in CompactTriangulation::reorderCells. Have you already noticed this behavior?

Thanks,
Pierre

julien-tierny · 2021-12-06T15:16:43Z

@guoxiliu:
could you have a look at merging @pierre-guillou 's patch?

also, it seems that the memory issue @pierre-guillou mentioned seems to be the last obstacle to the merge of this PR.
this would be great if you could have a look into it.

thanks for your feedback.
best,

guoxiliu · 2021-12-06T19:06:53Z

Thank you, @julien-tierny and @pierre-guillou. I am working on merging the patch and the memory issue. I will update the repo when I fix it.

Best,
Guoxi

Format the code

pierre-guillou · 2021-12-08T13:25:44Z

Thanks @guoxiliu! The memory issue seems to be gone with your last commit!
Everything looks good to me.
Looking forward to the merge!

julien-tierny · 2021-12-13T15:57:17Z

Alright, looks good to me.
Let's go then!

julien-tierny · 2021-12-13T15:58:47Z

Now @guoxiliu
I will ask you one more thing:
can you checkout our contributor guide to ttk-data and PR ttk-data to provide an example of usage of the compact triangulation?

guoxiliu · 2021-12-13T17:58:11Z

Alright, looks good to me. Let's go then!

Thank you, @julien-tierny!

Now @guoxiliu I will ask you one more thing: can you checkout our contributor guide to ttk-data and PR ttk-data to provide an example of usage of the compact triangulation?

I will follow the guideline in the link you mentioned and prepare the example.

Guoxi and others added 5 commits July 31, 2021 16:27

Add TopoCluster data structure

6224591

Adapt the new data structure

ab83593

Use data and offsets vectors for FlatJaggedArray

3fbc65b

Rename the data structure and plugin

29fe619

Add the global cache size variable

Merge branch 'topology-tool-kit:dev' into dev

45414e6

guoxiliu force-pushed the dev branch from 450ac64 to da20964 Compare October 26, 2021 19:37

Fix warnings and build errors

2841fd9

guoxiliu force-pushed the dev branch from da20964 to 2841fd9 Compare October 27, 2021 03:35

Format the code and fix warnings

bd12d55

guoxiliu force-pushed the dev branch from e1f7deb to bd12d55 Compare October 27, 2021 15:39

julien-tierny requested changes Oct 31, 2021

View reviewed changes

julien-tierny and others added 4 commits October 31, 2021 08:32

Merge branch 'dev' of https://github.com/topology-tool-kit/ttk into dev

a96877d

re-order paraview gui items

48446e9

Get rid of pointers

6c87420

Update the reference in vtk header and xml file

Try fixing the build error

d4019e1

julien-tierny approved these changes Nov 14, 2021

View reviewed changes

julien-tierny requested changes Nov 14, 2021

View reviewed changes

guoxiliu force-pushed the dev branch from 430334a to 06561f4 Compare November 18, 2021 19:28

Remove dynamic allocation

989e867

Move Octree to the base folder Remove debug message prefix in vtk file

guoxiliu force-pushed the dev branch from 06561f4 to 989e867 Compare November 18, 2021 20:22

julien-tierny approved these changes Nov 19, 2021

View reviewed changes

julien-tierny requested changes Nov 19, 2021

View reviewed changes

core/base/octree/Octree.cpp Outdated Show resolved Hide resolved

core/base/octree/Octree.cpp Outdated Show resolved Hide resolved

core/base/octree/Octree.h Outdated Show resolved Hide resolved

Remove dynamic allocation from Octree

6b4efc8

julien-tierny approved these changes Nov 21, 2021

View reviewed changes

switch from dataset to pointset

7e7a685

julien-tierny reviewed Nov 21, 2021

View reviewed changes

paraview/xmls/CompactTriangulationPreconditioning.xml Outdated Show resolved Hide resolved

pierre-guillou reviewed Nov 22, 2021

View reviewed changes

Fix several issues and organize the code

2fcd332

guoxiliu force-pushed the dev branch from 9a05bea to 2fcd332 Compare December 1, 2021 06:58

julien-tierny added the discussion Ongoing discussion prior to merge label Dec 6, 2021

Pierre Guillou and others added 4 commits December 6, 2021 17:39

Reduce variables scope

8c69623

Clear state before computation?

60d3700

Call parent class copy constructor

a6cd3ce

Initialize the variable of total clusters

62e09a8

Format the code

julien-tierny merged commit 518dabb into topology-tool-kit:dev Dec 13, 2021

julien-tierny mentioned this pull request Jan 13, 2022

Compact Triangulation Cache Size Everywhere #712

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compact Triangulation Integration #674

Compact Triangulation Integration #674

guoxiliu commented Oct 25, 2021

julien-tierny commented Oct 27, 2021

guoxiliu commented Oct 27, 2021

julien-tierny left a comment

julien-tierny left a comment

julien-tierny left a comment

julien-tierny commented Nov 21, 2021

julien-tierny commented Nov 21, 2021

pierre-guillou left a comment

pierre-guillou Nov 22, 2021

pierre-guillou Nov 22, 2021

pierre-guillou Nov 22, 2021

pierre-guillou Nov 22, 2021

pierre-guillou Nov 22, 2021

pierre-guillou Nov 22, 2021

pierre-guillou Nov 22, 2021

pierre-guillou Nov 22, 2021

pierre-guillou Nov 22, 2021

pierre-guillou Nov 22, 2021

julien-tierny commented Dec 2, 2021

pierre-guillou commented Dec 3, 2021

julien-tierny commented Dec 6, 2021

guoxiliu commented Dec 6, 2021

pierre-guillou commented Dec 8, 2021

julien-tierny commented Dec 13, 2021

julien-tierny commented Dec 13, 2021 •

edited

Loading

guoxiliu commented Dec 13, 2021

		std::vector<std::string>::iterator iter = scalarFields.begin();
		while(iter != scalarFields.end()) {

		iter = scalarFields.begin();
		while(iter != scalarFields.end()) {

Compact Triangulation Integration #674

Compact Triangulation Integration #674

Conversation

guoxiliu commented Oct 25, 2021

julien-tierny commented Oct 27, 2021

guoxiliu commented Oct 27, 2021

julien-tierny left a comment

Choose a reason for hiding this comment

julien-tierny left a comment

Choose a reason for hiding this comment

julien-tierny left a comment

Choose a reason for hiding this comment

julien-tierny commented Nov 21, 2021

julien-tierny commented Nov 21, 2021

pierre-guillou left a 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

julien-tierny commented Dec 2, 2021

pierre-guillou commented Dec 3, 2021

julien-tierny commented Dec 6, 2021

guoxiliu commented Dec 6, 2021

pierre-guillou commented Dec 8, 2021

julien-tierny commented Dec 13, 2021

julien-tierny commented Dec 13, 2021 • edited Loading

guoxiliu commented Dec 13, 2021

julien-tierny commented Dec 13, 2021 •

edited

Loading