-
Notifications
You must be signed in to change notification settings - Fork 124
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
add Topological Optimization code #1043
Conversation
Hi Mohamed, 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 ( To know in your C++ code if Torch has been found or not, use the preprocessor instruction Please proceed to this change and fix any issue until the CI succeeds. |
…opologicalOptimization
|
There was a problem hiding this 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> |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to remove.
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. |
Hi Mohamed, thanks a lot for your edits.
|
Moreover, the input data must be first normalized in a pre-process, and "de-normalized" in a post-process. |
Also, please report a status message every ten iterations (and not every iterations). Thanks! |
…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.
hi mohamed, 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). |
…f the target diagram.
OK, looks good to me. Thanks a lot! |
Integration of the code for topological simplification based on persistence diagram optimization.