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.

Sign up for GitHub

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

Jump to bottom

Test insertion of simplex with NaN filtration value #424

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Test insertion of simplex with NaN filtration value #424

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73c8001
Test insertion of simplex with NaN filtration value
mglisse Nov 5, 2020
52a5406
Slightler longer test
mglisse Nov 5, 2020
767f8cb
More doc.
mglisse Nov 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/Simplex_tree/include/gudhi/Simplex_tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,9 +763,11 @@ class Simplex_tree {
return insert_vertex_vector(copy, filtration);
}

/** \brief Insert a N-simplex and all his subfaces, from a N-simplex represented by a range of
/** \brief Insert a N-simplex and all its subfaces, from a N-simplex represented by a range of
* Vertex_handles, in the simplicial complex.
*
* Inserting a simplex with filtration value NaN does not modify the filtration value of any pre-existing simplex.
*
* @param[in] Nsimplex range of Vertex_handles, representing the vertices of the new N-simplex
* @param[in] filtration the filtration value assigned to the new N-simplex.
* @return If the new simplex is inserted successfully (i.e. it was not in the
Expand Down Expand Up @@ -1352,7 +1354,8 @@ class Simplex_tree {
* filtration values.
* @return True if any filtration value was modified, false if the filtration was already non-decreasing.
*
* If a simplex has a `NaN` filtration value, it is considered lower than any other defined filtration value.
* If a simplex of dimension 1 or more has a `NaN` filtration value,
* it is considered lower than any other defined filtration value.
*/
bool make_filtration_non_decreasing() {
bool modified = false;
Expand All @@ -1379,6 +1382,7 @@ class Simplex_tree {
for (auto& simplex : boost::adaptors::reverse(sib->members())) {
// Find the maximum filtration value in the border
Boundary_simplex_range boundary = boundary_simplex_range(&simplex);
// We would need a loop using fmax to get consistent results when a vertex has NaN.
Boundary_simplex_iterator max_border = std::max_element(std::begin(boundary), std::end(boundary),
[](Simplex_handle sh1, Simplex_handle sh2) {
return filtration(sh1) < filtration(sh2);
Expand Down
10 changes: 10 additions & 0 deletions src/python/gudhi/simplex_tree.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,11 @@ cdef class SimplexTree:
simplices are already present with a higher filtration value, their
filtration value is lowered.

.. note::

Inserting a simplex with filtration value `math.nan` does not
modify the filtration value of any simplex already present.
Copy link
Contributor

Choose a reason for hiding this comment

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

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().

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


:param simplex: The N-simplex to insert, represented by a list of
vertex.
:type simplex: list of int
Expand Down Expand Up @@ -368,6 +373,11 @@ cdef class SimplexTree:
"""This function ensures that each simplex has a higher filtration
value than its faces by increasing the filtration values.

.. note::

For this function, a value of NaN for a simplex of dimension 1 or more
is considered smaller than any other value.

:returns: True if any filtration value was modified,
False if the filtration was already non-decreasing.
:rtype: bool
Expand Down
15 changes: 15 additions & 0 deletions src/python/test/test_simplex_tree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

from gudhi import SimplexTree
import pytest
import math

__author__ = "Vincent Rouvreau"
__copyright__ = "Copyright (C) 2016 Inria"
Expand Down Expand Up @@ -399,3 +400,17 @@ def test_boundaries_iterator():

with pytest.raises(RuntimeError):
list(st.get_boundaries([6])) # (6) does not exist

# insert with NaN must not modify filtration values
# make_filtration_non_decreasing considers NaN small
def test_nan():
st = SimplexTree()
st.insert([1], 2.)
st.insert([4], 5.)
st.insert([2], -1.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Member Author

Choose a reason for hiding this comment

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

I inserted an extra edge.

st.insert([1, 2, 4], math.nan)
assert math.isnan(st.filtration([2, 4]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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().

Copy link
Member Author

Choose a reason for hiding this comment

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

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.make_filtration_non_decreasing()
assert st.filtration([1, 2, 4]) == 5.
assert st.filtration([1, 2]) == 2.
assert st.filtration([2]) == -1.