First commit for Dictionary #948

Keanu-Sisouk · 2023-06-29T16:06:56Z

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:

Your description here

Keanu-Sisouk · 2023-06-30T14:52:46Z

Code corrected for format, lint code and warnings checking

…nt code

…sh_back

julien-tierny · 2023-10-30T06:11:10Z

Can you please sync to resolve conflicts? Thanks!

Keanu-Sisouk · 2023-10-30T10:39:21Z

Branch rebased on ttk-dev and conflicts resolved (the numerical loss returned may change a little bit)

…gramDictionary Module

Keanu-Sisouk · 2023-10-30T14:57:59Z

Correcting PersistenceDiagramClustering/PDBarycenter.cpp line 346 for Error in Check-Code and Check-Build

julien-tierny

hi keanu,
thanks a lot for this PR!
here's a first batch of comments.

essentially, they are mostly about naming conventions (for variables and functions, some clever sed -- i.e. find/replace -- should do)
a few clarifications regarding Eigen
some unnecessary headers to remove
so signal creation to remove
comments to remove, etc.
feel free to ping me on slack if you need more info!
thanks :)

julien-tierny · 2023-12-18T13:35:28Z

core/base/constrainedGradientDescent/CMakeLists.txt

+    ConstrainedGradientDescent.h
+  DEPENDS
+    common
+    persistenceDiagram


do you need this?
if so, the name of the class may have to be changed.
(if it is specialized for persistence diagrams, the name of the class should reflect it).

maybe you'd need to rename it into something like PersistentDiagramConstrainedOptimization

or, you make this class more general (and not specialized to persistence diagram). to discuss I guess.

julien-tierny · 2023-12-19T08:30:04Z

core/base/initDictPersistenceDiagram/InitDictPersistenceDiagram.h

+namespace ttk {
+  using Matrix = std::vector<std::vector<double>>;
+
+  class InitDictPersistenceDiagram : public Debug {


I find the name of the class not very explicit.
Words are usually not abbreviated in TTK.
I'd suggest to change the name of the class (and the related files) to something like PersistenceDiagramDictionaryInitializer (PersistenceDictionaryInitializer)

julien-tierny · 2023-12-19T08:32:33Z

core/base/persistenceDiagramDictionaryDecoding/CMakeLists.txt

+    common
+    persistenceDiagram
+    persistenceDiagramClustering
+    persistenceDiagramDistanceMatrix


are all these entries needed? (it's easy to try, just remove these lines and see if it sill works).
I have the impression that the dictionary already depend on the distanceMatrix, so I wonder if these lines are really needed. To try.

julien-tierny · 2024-01-22T09:22:09Z

core/base/constrainedGradientDescent/ConstrainedGradientDescent.h

+      std::vector<std::vector<std::array<double, 2>>> &pairToAddGradList,
+      ttk::DiagramType &infoToAdd);
+
+    void setStep(double factEquiv);


-> double &factEquiv (also in the implementation of the function).
use & by default

julien-tierny · 2024-01-22T09:25:04Z

core/base/constrainedGradientDescent/ConstrainedGradientDescent.h

+      std::vector<std::vector<std::array<double, 2>>> &pairToAddGradList,
+      ttk::DiagramType &infoToAdd);
+
+    double stepAtom;


-> stepAtom_
for member variables (i.e. member of a class), the convention in ttk is to put an extra _ at the end of the name (to quickly differentiate those from local variables, defined only within functions)

(... except in the specific case of variables directly manipulated by VTK getters and setters)

julien-tierny · 2024-01-22T16:09:56Z

core/vtk/ttkPersistenceDiagramDictionary/ttkPersistenceDiagramDictionary.cpp

+#include <vtkDoubleArray.h>
+#include <vtkFiltersCoreModule.h>
+#include <vtkFloatArray.h>
+#include <vtkIntArray.h>


that include does not seem necessary.
please remove all unnecessary includes (i.e. delete the line and see if it still builds)

julien-tierny · 2024-01-22T16:10:37Z

core/vtk/ttkPersistenceDiagramDictionary/ttkPersistenceDiagramDictionary.h

+  double Percent_{0};
+
+public:
+  // enum class BACKEND{BORDER_INIT = 0 , RANDOM_INIT = 1 , FIRST_DIAGS = 2};


remove comment if not needed

julien-tierny · 2024-01-22T16:13:24Z

core/vtk/ttkPersistenceDiagramDictionaryDecoding/ttkPersistenceDiagramDictionaryDecoding.h

+/// \author Pierre Guillou <pierre.guillou@lip6.fr>
+/// \date Mai 2023
+///
+/// \brief TTK processing package for the computation of a Dictionary


include proper class description.

julien-tierny · 2024-01-22T16:14:30Z

core/vtk/ttkPersistenceDiagramDictionaryDecoding/ttkPersistenceDiagramDictionaryDecoding.h

+
+  int FillOutputPortInformation(int port, vtkInformation *info) override;
+
+  void outputDiagrams(vtkMultiBlockDataSet *output,


in the VTK layer, the function names should probably match VTK's naming convention (GetOutputDiagrams())

julien-tierny · 2024-01-22T16:15:07Z

core/vtk/ttkPersistenceDiagramDictionaryDecoding/ttkPersistenceDiagramDictionaryDecoding.h

+                      const double spacing,
+                      const double maxPersistence) const;
+
+  double getMaxPersistence(const ttk::DiagramType &diagram) const;


in the VTK layer, the function names should probably match VTK's naming convention (cf above), plus, make the function name more explicit.

julien-tierny · 2024-02-05T13:51:49Z

hi keanu,
I've been running the example that you prepared for ttk-data (topology-tool-kit/ttk-data#151).
During the iterations of the optimization, it seems that the timing for the substeps is incorrect (always reporting 12 seconds, no mater what). for instance:

[PersistenceDiagramDictionary] Weight Optimization activated
[PersistenceDiagramDictionary] Atom Optimization activated
[PersistenceDiagramDictionary] Minimum not passed273.783325 ..... [12.000s|100%]
[PersistenceDiagramDictionary]  Epoch 32, loss = 273.783325 ..... [12.000s|100%]
[PersistenceDiagramDictionary] Loss returned 206.159905 at Epoch 15
[PersistenceDiagramDictionary] Complete ...................... [0.029s|12T|100%]
[PersistenceDiagramDictionary] New step multi-scale approach:
[PersistenceDiagramDictionary] Weight Optimization activated
[PersistenceDiagramDictionary] Atom Optimization activated
[PersistenceDiagramDictionary] Loss not decreasing enough87 ..... [12.000s|100%]
[PersistenceDiagramDictionary]  Epoch 23, loss = 290.404787 ..... [12.000s|100%]
[PersistenceDiagramDictionary] Loss returned 251.300186 at Epoch 49
[PersistenceDiagramDictionary] Complete ...................... [0.037s|12T|100%]
[PersistenceDiagramDictionary] New step multi-scale approach:
[PersistenceDiagramDictionary] Weight Optimization activated
[PersistenceDiagramDictionary] Atom Optimization activated
[PersistenceDiagramDictionary] Minimum not passed638.536093 ..... [12.000s|100%]
[PersistenceDiagramDictionary]  Epoch 32, loss = 638.536093 ..... [12.000s|100%]
[PersistenceDiagramDictionary] Loss returned 567.337566 at Epoch 71
[PersistenceDiagramDictionary] Complete ...................... [0.197s|12T|100%]
[PersistenceDiagramDictionary] New step multi-scale approach:
[PersistenceDiagramDictionary] Weight Optimization activated
[PersistenceDiagramDictionary] Atom Optimization activated
[PersistenceDiagramDictionary] Loss not decreasing enough174 .... [12.000s|100%]
[PersistenceDiagramDictionary]  Epoch 24, loss = 1064.678174 .... [12.000s|100%]
[PersistenceDiagramDictionary] Loss returned 1052.556453 at Epoch 108
[PersistenceDiagramDictionary] Complete ...................... [0.542s|12T|100%]
[PersistenceDiagramDictionary] Final step multi-scale approach:
[PersistenceDiagramDictionary] Weight Optimization activated
[PersistenceDiagramDictionary] Atom Optimization activated
^[[B[PersistenceDiagramDictionary]  Epoch 14, loss = 1782.946038 .... [12.000s|1[PersistenceDiagramDictionary] Minimum not passed1645.441462 .... [12.000s|100%]
[PersistenceDiagramDictionary]  Epoch 64, loss = 1645.441462 .... [12.000s|100%]
[PersistenceDiagramDictionary] Loss returned 1603.601491 at Epoch 163

also, some undesired debug messages are still visible in the command line:

[PersistenceDiagramDictionary] Complete ..................... [95.281s|12T|100%]
[PersistenceDiagramDictionary] Total time ................... [96.087s|12T|100%]
[PersistenceDiagramDictionaryDecoding] %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
Atom0
Atom1
Atom2
ClusterID
TimeStep
FILE
[PersistenceDiagramDictionaryDecoding] Computed barycenters .. [2.046s|12T|100%]

Thanks for fixing these (tiny) issues.

julien-tierny

here's a few extra comments after running your state file.
thanks!

julien-tierny · 2024-02-05T14:01:04Z

paraview/xmls/PersistenceDiagramDictionary.xml

+
+
+      <!-- Create a UI group that contains all output parameter widgets (here only one) -->
+      <PropertyGroup panel_widget="Line" label="Input Options">


These are more "Output Options"

julien-tierny · 2024-02-05T14:03:47Z

paraview/xmls/PersistenceDiagramDictionary.xml

+
+      <!-- Create a UI group that contains all output parameter widgets (here only one) -->
+      <PropertyGroup panel_widget="Line" label="Input Options">
+        <Property name="AtomNumber_" />


Thanks for listing all the other options here too.
Also:

please organize and group them by theme

make the labeling convention more uniform (the first word only starts with a capital)

remove testing options

make the labels more explicit

extend the option documentation (<Documentation>...</Documentation>)

julien-tierny · 2024-02-05T14:04:37Z

paraview/xmls/PersistenceDiagramDictionaryDecoding.xml

+      <!-- A string parameter that controls the name of the output array -->
+
+
+      <!-- Create a UI group that contains all output parameter widgets (here only one) -->


please add a category "Output Options" (like for the PersistenceDiagramDictionary filter)

julien-tierny · 2024-02-05T14:50:50Z

paraview/xmls/PersistenceDiagramDictionaryDecoding.xml

+
+      <!-- Create a UI group that contains all output parameter widgets (here only one) -->
+
+


Does this filter (at least in principle) depends on some of the parameter values set up during encoding?
(for instance to control the size of the barycenters)
If so, these parameter values should be encoded as field data (and read during decoding)

Keanu-Sisouk force-pushed the dictionary branch 2 times, most recently from 1e6ac8d to 51db9f0 Compare September 22, 2023 14:17

Keanu Sisouk added 3 commits October 20, 2023 14:26

First commit for Dictionary

500983b

[InitDictPersDiagram base] correction, passing by reference for CI li…

4fa6e61

…nt code

[InitDictPersDiagram base] correction, deleting temp2 and directly pu…

2abf2b1

…sh_back

Keanu-Sisouk force-pushed the dictionary branch from 51db9f0 to 2abf2b1 Compare October 30, 2023 10:37

[PersistenceDiagramClustering] Correction line 346 for PersistenceDia…

34247e8

…gramDictionary Module

julien-tierny requested changes Jan 22, 2024

View reviewed changes

julien-tierny requested changes Feb 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First commit for Dictionary #948

First commit for Dictionary #948

Keanu-Sisouk commented Jun 29, 2023

Keanu-Sisouk commented Jun 30, 2023

julien-tierny commented Oct 30, 2023

Keanu-Sisouk commented Oct 30, 2023

Keanu-Sisouk commented Oct 30, 2023

julien-tierny left a comment

julien-tierny Dec 18, 2023

julien-tierny Dec 19, 2023

julien-tierny Dec 19, 2023

julien-tierny Dec 19, 2023

julien-tierny Dec 19, 2023

julien-tierny Jan 22, 2024

julien-tierny Jan 22, 2024

julien-tierny Jan 22, 2024

julien-tierny Jan 22, 2024

julien-tierny Jan 22, 2024

julien-tierny Jan 22, 2024

julien-tierny Jan 22, 2024

julien-tierny Jan 22, 2024

julien-tierny commented Feb 5, 2024

julien-tierny left a comment

julien-tierny Feb 5, 2024

julien-tierny Feb 5, 2024

julien-tierny Feb 5, 2024

julien-tierny Feb 5, 2024


		int FillOutputPortInformation(int port, vtkInformation *info) override;

		void outputDiagrams(vtkMultiBlockDataSet *output,



		<!-- Create a UI group that contains all output parameter widgets (here only one) -->
		<PropertyGroup panel_widget="Line" label="Input Options">

		<!-- A string parameter that controls the name of the output array -->


		<!-- Create a UI group that contains all output parameter widgets (here only one) -->

First commit for Dictionary #948

Are you sure you want to change the base?

First commit for Dictionary #948

Conversation

Keanu-Sisouk commented Jun 29, 2023

Keanu-Sisouk commented Jun 30, 2023

julien-tierny commented Oct 30, 2023

Keanu-Sisouk commented Oct 30, 2023

Keanu-Sisouk commented Oct 30, 2023

julien-tierny 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien-tierny commented Feb 5, 2024

julien-tierny 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