search

topology-tool-kit / ttk

TTK - Topological Data Analysis and Visualization - Source Code
https://topology-tool-kit.github.io/
Other
415 stars 124 forks source link

Added fix for joinTree not inverting the data order back #986

Closed m-s-will closed 11 months ago

m-s-will commented 11 months ago

There was a bug when computing the join tree with ExTreeM. By default it computes the split tree and it can be adapted into computing the join tree by inverting the order values of the data. However, the values weren't inverted back, leading to wrong values in further steps. This PR fixes this.

julien-tierny commented 11 months ago

Hi @m-s-will, thanks a lot for this quick fix. I had indeed observed it and I was meaning to open an issue. I have a few remarks/questions

You're modifying in place the array orderArrayData. Is that a local copy of the order array? (to double-check with @JonasLukasczyk ) If not, my understanding is that this will affect the other filters down the pipeline and the order will be reverted there too, which is not desirable.

Also, I observed that if you apply the filter "TTKPathCompression" first in the pipeline, the "TTKMergeTree" filter (with ExTreeM enabled) will still recompute the path compression (and show the message, "TIP: Call TTKPathCompression first for improved performance"). It looks like something is incorrect there.

Finally, "TTKPathCompression" provides data arrays (for the segmentations) whose names do not include the name of the corresponding scalar field as a prefix. I believe this should be the case (just like the OrderArray).

JonasLukasczyk commented 11 months ago

The current code avoids such huge data copies and instead modifies the data in-place. But of course these changes always need to be reverted or this will lead to bugs (as happened here). Btw., LTS follows the same strategy. One also needs to be aware that if the order is reverted and then an error occurs, the order sill needs to be reverted back or the next execution will not work! Please check if you do that @m-s-will.

m-s-will commented 11 months ago

Hello Jonas and Julien,

@JonasLukasczyk thanks for the reminder in case of an error, it is now reverted regardless of the base execution running without errors. @julien-tierny : I adapted PathCompression to create a naming similarly to the order array, e.g. arrayName_AscendingSegmentation and arrayName_DescendingSegmentation. MergeTree looks for these names, it was looking for the wrong names previously and therefore always reran the PC step.

julien-tierny commented 11 months ago

ok, looks good. thanks!