-
Notifications
You must be signed in to change notification settings - Fork 31
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
Path Mappings and Branch Mappings in MergeTreeBarycenter and MergeTreeDistanceMatrtix #162
base: dev
Are you sure you want to change the base?
Conversation
Thanks a lot Florian! Thanks a lot! |
A quick question regarding the docs: where are the screenshots supposed to be saved? They are referenced in the other markdown files, but as far as I can see they are not present in the data repository and the references point to the ttk website. |
hi florian, thanks for your message. |
ok, thanks! so they will be generated automatically? |
not exactly, but we can generate them after the fact. |
Ok, sounds reasonable. One more question: could you have a look at the current python states? Since the output of the MergeTreeClustering is a vtk multiblock, I had to save the barycenter and mapping in that format, which is not very nice in my opinion. Would you prefer to keep it like this or to add some code that extracts the unstructured grids? I am not sure if we should prefer a cleaner output or the simplicity of the python code. (I think csv outputs are not reasonable here, as not all scalars seem to be saved.) |
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.
here's a quick review.
- [outlier.vtm](https://github.com/topology-tool-kit/ttk-data/tree/dev/bdied_outlier/outlier.vtm): a vtk multiblock containing 10 regular grids. | ||
|
||
## Outputs | ||
- `merge_tree_barycenter.vtm`: the computed barycenter merge tree as a vtk multiblock. |
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.
It looks like the first output is the mapping (based on the above text and the python script).
Is that correct?
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.
Yes, the description wasn't correct here. The file name was wrong, too. I fixed it now. Thanks!
Good point, I will try some different setups. I think the layout parameters won't do a lot, but I could try extracting different members of the ensemble. |
Maybe you can try to play with the "Important Pair Threshold"? (to make the front subtree switch from the right to the left side) |
Yes that worked, I have pushed the updates. |
Another question regarding the outputs of the python scripts: Are they just for testing/CI purposes or should they also yield nice renderings? Because at the moment I only went for objects that are useful for verifying the output, e.g. only the mapping in the branch mapping example. |
for the python script, I guess the idea is to output something that is useful for an analysis in batch mode (irrespective of the actual visualization). the barycenter (or the matchings) and the distance matrices are good in that perspective. |
Thanks for the quick updates! For the branchMapping example, there's something that is not clear to me. |
Yes, that's correct. I think it's always the first two trees, but maybe @MatPont knows more. The same behavior is applied for the original Merge Tree Edit Distance, or in general if the I chose to keep the whole ensemble to have a unified input for the two BDIED examples. I also think it's a good way to showcase how the filter operates without the Do you want to keep the example as is or do you want me to change it? |
I think the example is good as is, but it'd be nice to clarify the behavior of the filter in the documentation (i.e., for these backends which do not support barycenter computation, only a matching between the first two trees will be computed). |
Shouldn't it be formulated differently? This behavior happens for all distances if the computeBarycenter flag is not set. You can check that in the state file by first selecting a supported distance and then setting the flag to false. Then you get the same behavior with any distance. I think at the moment, if the user want to compute a barycenter with an unsupported distance, the output is simply empty and an error message is printed to console. The parameters are no longer corrected automatically. This is actually something that @MatPont wanted to discuss with you, as we run into some weird workflows when deactivating specific parameter combinations. |
Hi @floWetzels , sorry it took me some time to come back to this review.
Thanks a lot for your feedback! |
The same seems to happen for the path mapping backend... |
Hi @julien-tierny, good catch! Regarding the field data, I am really confused. I didn't even know about that feature and never touched it. For me, the problem actually appears for all four distances. I had a quick look at the vtk layer code and didn't find anything odd. Could be that something is wrong with the parameters in the state file, it uses two different filters after all. But I couldn't find anything there neither... @MatPont Do you have any idea on this? Regarding the cost array, I think this was a deliberate choice. I don't remember it exactly, but I think it had something to do with the incompatible matching types and their conversion. I am not sure though, could also be a bug. I you want me to, I could have a look next week. I don't know if I'll find the time during EuroVis, but I can try. Just let me know. |
hi @floWetzels , |
Hi @julien-tierny, |
After discussing with @floWetzels it seems that all current todos are done, and the problems discussed in this PR are fixed except one thing that @floWetzels will take care of. Basically, the cost array in the matching output for the branch mapping backend is not filled for the interior nodes (it is only for the leaves and the root for the moment), their values should be copied from the corresponding leaf of their branch. |
thanks a lot for the feedback! |
I think all issues are fixed now. Can you have another look, @julien-tierny? |
This PR adds state and data files for the path mapping distance and branch mapping distance used in the MergeTreeBarycenter and MergeTreeDistanceMatrix modules. The examples come from the VIS23 paper "Merge Tree Geodesics and Barycenters with Path Mappings".
It accompanies the following TTK pull request: topology-tool-kit/ttk#1025