Test insertion of simplex with NaN filtration value #424
Conversation
There was a problem hiding this comment.
From my (biased) perspective, it seems to be a nice addition. Perhaps it is almost useless (I don't really know how people use gudhi in general), but when you typically want to build a lower start filtration, starting from a complex + values on its vertices, this might simplify things a bit.
| st.insert([4], 5.) | ||
| st.insert([2], -1.) | ||
| st.insert([1, 2, 4], math.nan) | ||
| assert math.isnan(st.filtration([2, 4])) |
There was a problem hiding this comment.
| assert math.isnan(st.filtration([2, 4])) | |
| assert math.isnan(st.filtration([2, 4])) | |
| assert st.filtration([4]) == 5. | |
| assert st.filtration([2]) == -1. | |
| assert st.filtration([1]) == 2. | |
| assert st.filtration([1,2]) == 2. |
Perhaps we may want to make sure that we did not modify any of the previous values even before calling st.make_persistence_non_decreasing().
There was a problem hiding this comment.
I thought that checking after make_persistence_non_decreasing would be sufficient (if we overwrite a value with insert, make_persistence_non_decreasing won't be able to reinvent that value), but why not, done.
| st = SimplexTree() | ||
| st.insert([1], 2.) | ||
| st.insert([4], 5.) | ||
| st.insert([2], -1.) |
There was a problem hiding this comment.
| st.insert([2], -1.) | |
| st.insert([2], -1.) | |
| st.insert([1,2], 2.) |
Perhaps already add a single edge (with an admissible value) to make sure that both inserting nan and make_filtration_non_decreasing() do not play any role there
There was a problem hiding this comment.
I inserted an extra edge.
| .. note:: | ||
|
|
||
| Inserting a simplex with filtration value `math.nan` does not | ||
| modify the filtration value of any simplex already present. |
There was a problem hiding this comment.
Perhaps complete this note by adding a warning in the vein of what is done for assign_filtration() saying (e.g.) "In this particular case, the result will not be a valid filtration anymore. Callers are responsible for fixing this (using assign_filtration() or make_filtration_non_decreasing() for instance) before calling any function that relies on the filtration property, like persistence().
There was a problem hiding this comment.
I hadn't realized it, but having some NaN in the complex can have some weird effects. In particular, if I have vertex 0 with value NaN, inserting edge [0, 1] with value 42 does not change the value of vertex 0. I added some confusing text to the doc. We could change that behavior, but would need to check that it doesn't slow things down for normal operations.
|
It would be nice to test it at C++ level. Here is a gist that translates the python test in C++. |
This patch documents (and tests) that inserting simplices with filtration value NaN does not modify the value for existing simplices.
Now to discuss whether that's actually the behavior we want 😉