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

add Topological Optimization code #1043

Merged

Conversation

MohamedKISSI
Copy link
Contributor

@MohamedKISSI MohamedKISSI commented Jul 4, 2024

Integration of the code for topological simplification based on persistence diagram optimization.

@julien-tierny
Copy link
Collaborator

julien-tierny commented Jul 5, 2024

Hi Mohamed,
thanks a lot for this PR.

At this point, the PR doesn't pass the continuous integration (CI), which stopped on a third-party dependency issue (here, torch). As you will read in this documentation (https://github.com/topology-tool-kit/ttk/wiki/Guidelines-for-Developing-a-New-TTK-Module), TTK must build and run even when third-party dependencies haven't been found.

This means that your code should build and run, even if torch is not installed. In particular, if the optimization backend selected by the user was Adam and that Torch was not installed, you should display a warning (printWrn) explaining that Adam cannot be used because Torch hasn't been found and that the code will now default to direct gradient descent.

To know in your C++ code if Torch has been found or not, use the preprocessor instruction #ifdef TTK_ENABLE_TORCH (see https://github.com/topology-tool-kit/ttk/blob/dev/core/base/mergeTreeAutoencoder/MergeTreeAutoencoder.h for an example).

Please proceed to this change and fix any issue until the CI succeeds.
Thanks

@julien-tierny
Copy link
Collaborator

  • Code tested on a simple example (with and without Torch support, with multiple options)
  • PV GUI edited
  • base code refactored
  • console messages edited

Copy link
Collaborator

@julien-tierny julien-tierny left a comment

Choose a reason for hiding this comment

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

Hi Mohamed,
thanks again for this PR.
I did quite some modifications to it.

I left a question in my review as well as a request for removing yet another function.
thanks!

- neighborsIndices : vector which contains the neighboring vertices of
vertex i
*/
template <typename triangulationType>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove that function (see my comments below when it is used).

needUpdate[indice] = true;
// Find all the neighbors of the vertex
std::vector<SimplexId> neighborsIndices;
getNeighborsIndices(triangulation, indice, neighborsIndices);
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove that call.

std::vector<SimplexId> neighborsIndices;
getNeighborsIndices(triangulation, indice, neighborsIndices);

for(SimplexId neighborsIndice : neighborsIndices) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

replace that loop by:

int vertexNumber = triangulation->getVertexNeighborNumber(index);
for(int i = 0; i < vertexNumber; i++){
   int vertexNeighborId = -1;
   triangulation->getVertexNeighbor(index, i, vertexNeighborId);
   needUpdate[vertexNeighborId] = true;
}

vertex i
*/
template <typename triangulationType>
int ttk::TopologicalOptimization::getNeighborsIndices(
Copy link
Collaborator

Choose a reason for hiding this comment

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

to remove.

@julien-tierny julien-tierny added the WIP work in progress label Aug 23, 2024
@MohamedKISSI
Copy link
Contributor Author

Hi Mohamed, thanks again for this PR. I did quite some modifications to it.

I left a question in my review as well as a request for removing yet another function. thanks!

Hi Julien,

Thank you for your feedback. It's duly noted; I have removed the getNeighborsIndices function and changed the type of the dataVector variable to dataType.
Thanks

@julien-tierny
Copy link
Collaborator

julien-tierny commented Aug 27, 2024

Hi Mohamed,

thanks a lot for your edits.
We need to discuss a couple of things though:

  • necessity of double format for the data vector (see kFloat64), as a first effort, please use double for dataVectors and make all the computation in double.
  • check consistency between kFloat64 and kDouble
  • multiple unnecessary copies of the ouput data dataVector smoothedScalars

@julien-tierny
Copy link
Collaborator

Moreover, the input data must be first normalized in a pre-process, and "de-normalized" in a post-process.

@julien-tierny
Copy link
Collaborator

julien-tierny commented Aug 27, 2024

Also, please report a status message every ten iterations (and not every iterations). Thanks!
EDIT: make the value 10 editable from the GUI (with 10 being a default value)

…y between kFloat64 and kDouble, remove unnecessary copies of smoothedScalars. Normalize input data in pre-process and de-normalize in post-process. Report status message every k iterations.
@julien-tierny
Copy link
Collaborator

julien-tierny commented Aug 29, 2024

hi mohamed,
thanks for your quick fix.

it seems that your last commit is breaking things though.

please consider the statefile I sent you in slack, in the TopologicalSimplification node, switch from LTS to TopologicalOptimization and you'll see that the direct gradient descent method generates incorrect results (the adam method looks OK).

@julien-tierny
Copy link
Collaborator

OK, looks good to me. Thanks a lot!

@julien-tierny julien-tierny merged commit fcd1e70 into topology-tool-kit:dev Sep 3, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants