output squared filtration values true/false mechanism for Delaunay complexes #1172

VincentRouvreau · 2025-02-10T22:13:18Z

Replace #1138 (remove function instead of classes)
While keeping backward compatibilities of former Delaunay complexes (cpp, utils and python)

Fix #80 and #771

…mplexes

Test 3.13 with numpy 1.26.0

hschreiber · 2025-02-12T13:10:13Z

src/Alpha_complex/include/gudhi/Alpha_complex.h

+   * \note Weighted Alpha complex can have negative filtration values. If `output_squared_values` is set to `false`,
+   * filtration values will be `NaN` in this case.


If I understood correctly, contrary to the regular Alpha filtration, the filtration values in a weighted Alpha filtration are not distances and the option of having $\alpha$ squared or not has not much meaning for it. It would make more sense in that case to ignore output_squared_values. If the user did not want the filtration values (what the NaN represent, I guess?), he would just use a simplex tree with store_filtration at false, no?

Or are the NaN needed for some cases? Or did I simply misunderstood something?

In the weighted case, filtration values are the power distance of the smallest power sphere, and it can be a negative value. If it is asked to square root the filtration values, these will be NaN. I am ok with the idea to deactivate the square root mechanism in the weighted case. I will print a user warning in this case.

The power distance is not euclidean, from which the "square trick" is coming from. The option of squared or not squared feels off topic in this context. So, I would be more in the favor that we ignore this argument in the case of weighted alpha complexes (i.e., we do the same thing whatever its value). We could just state it like that in the documentation of the method, I am not sure a warning is needed then.
Except is there is some technical detail for which the NaN are helpful.

I did so on 44a6747, but I had to print a warning to the user in case he sets weights but don't want square filtration values.

hschreiber · 2025-02-12T13:15:56Z

src/Alpha_complex/include/gudhi/Alpha_complex.h

+    // For users to be able to define their own sqrt function on their desired Filtration_value type
+    using std::sqrt;


We could perhaps add in the concept of SimplicialComplexForAlpha that for "custom" filtration value types, having its own sqrt is possible and if not existing std::sqrt is used instead?

I specified it on 021561a

hschreiber · 2025-02-12T13:20:19Z

src/Alpha_complex/utilities/alpha_complex_persistence.cpp

@@ -29,8 +30,9 @@ using Simplex_tree = Gudhi::Simplex_tree<>;
 using Filtration_value = Simplex_tree::Filtration_value;

 void program_options(int argc, char *argv[], std::string &off_file_points, bool &exact, bool &fast,
-                     std::string &weight_file, std::string &output_file_diag, Filtration_value &alpha_square_max_value,
-                     int &coeff_field_characteristic, Filtration_value &min_persistence);
+                     bool& output_squared_root_values, std::string &weight_file, std::string &output_file_diag,


Shouldn't it be output_squared_values instead of output_squared_root_values? To be more consistent with the create_complex interface.

And is the correct term not square_root (without de d)? Or is it used as some sort of verb?

Edit: it seems that for the utilities in general (also for cech etc.), the option is inverted to the notion for the option. Was there a particular reason?

Yes, for the utilities the option is inverted because I wanted to keep backward compatibility.
Before this feature, it was returning squared values, so I wanted the utils to keep the behaviour.
And then the user can ask for square root filtration values with -s.

Does it seem ok for you ?

Ah, I just realized that this code will only be used precompiled with command lines, it is not a method to include. The only way to keep the old behaviour without inverting the argument would be to use -squared-filtrations=ON by default and explicity put -squared-filtrations=OFF instead of --square-root-filtrations in the command line arguments.

If this is too annoying to do, just keep it like that then. But I would still correct output_squared_root_values to output_square_root_values. Or be more explicit about the meaning of the option with do_not_square_filtration_values.

On this note, we could also change output_squared_values to use_squared_filtration_values / output_squared_filtration_values. As just value sounds a bit vague. But that is perhaps a bit subjective.

I did so on 44a6747, 4227322 and 404cb39

hschreiber · 2025-02-12T13:23:59Z

src/Alpha_complex/test/Weighted_alpha_complex_unit_test.cpp

+    }
+  }
+  std::clog << "Weighted alpha complex with output_squared_values\n";


Isn't it the other way around? "without output_squared_values", as create_complex<false> is used?

I fixed the log on a8d8634

src/Alpha_complex/utilities/alpha_complex_persistence.cpp

…y but to fit output_squared_filtration feature

…atibility but to fit output_squared_filtration feature

VincentRouvreau · 2025-02-14T16:07:31Z

@mglisse In the Weighted case, do you think we should always set output_squared_values to true ? As the distances can be negative, we would never have NaN values like this. This would be another way of coding the feature that may be (or not) less confusing for the end-user, no ?

…root_filtrations_assert_weighted

…ted case + some tests

VincentRouvreau · 2025-02-17T16:24:09Z

@hschreiber I took your proposal on 4ee4878 to always set output_squared_values to true in the weighted case, I just throw an exception otherwise. Does it fit what you were proposing ? I tried to static_assert, but it doesn't fit our need with the python interface. I could also ignore the user's choice in this case, but exceptions are quite convenient in Python.

hschreiber · 2025-02-17T17:00:00Z

src/Alpha_complex/include/gudhi/Alpha_complex.h

+   * \exception std::invalid_argument In case of a Weighted Alpha complex with `Output_squared_values` set to `false`,
+   * as Weighted Alpha complex can have negative filtration values.


Radical, but I am fine with the new proposition (even though, the value should be the other way around, as the filtration value is not squared (couldn't be negative otherwise), but technically it would not fit with the default value of the non weighted case). But here I would write "..., as the filtration values of a Weighted Alpha complex results from a power product and not from an euclidean distance. Therefore this template argument should be kept to the default value." instead of "..., as Weighted Alpha complex can have negative filtration values."

I am fine with your proposal. On bc7fbb6

mglisse · 2025-02-19T14:17:26Z

@mglisse In the Weighted case, do you think we should always set output_squared_values to true ? As the distances can be negative, we would never have NaN values like this. This would be another way of coding the feature that may be (or not) less confusing for the end-user, no ?

If a user knows they always have positive weights and wants the square root, it seems a bit disappointing to say NO when it would actually work just fine.

mglisse · 2025-02-19T14:23:05Z

src/python/gudhi/delaunay_complex.pyx

+            filtration: Set this value to `None` (default value) if filtration values are not needed to be computed
+                (will be set to `NaN`). Set it to `alpha` to compute the filtration values with the Alpha complex, or
+                to `cech` to compute the Delaunay Cech complex.
+            output_squared_values: Square filtration values when `True`. Default is `True`.


Does the Python doc mention somewhere that passing False can be dangerous with negative weights?

hschreiber · 2025-02-19T14:39:27Z

@mglisse In the Weighted case, do you think we should always set output_squared_values to true ? As the distances can be negative, we would never have NaN values like this. This would be another way of coding the feature that may be (or not) less confusing for the end-user, no ?

If a user knows they always have positive weights and wants the square root, it seems a bit disappointing to say NO when it would actually work just fine.

But what is the meaning of a "square rooted filtration value" of a weighted alpha complex? Would we return something like ||x-y||² - w when output_squared_values is true and ||x-y|| - w when false? Or sqrt(||x-y||² - w) when positive? I don't even know if CGAL makes it possible. In both cases, I don't feel like it is Gudhi's responsability to handle how users want to post-process the power product when there is no clear convention for it. Or is there?

VincentRouvreau and others added 9 commits February 10, 2025 11:01

Test 3.13 with numpy 1.26.0

70a4969

Try numpy<2 instead of explicit version + uplaod wheel before test

783d26a

output squared filtration values true/false mechanism for Delaunay co…

3d0e4f2

…mplexes

Forgotten modif

d80da71

Add some traces to debug

8e15b84

fix some codespell

93140af

Try with numpy 2.1 for python 3.13

ce04bc7

Restore python 3.9 as default python version for build

28adba1

Merge pull request GUDHI#1171 from VincentRouvreau/pip_cp313_ws_osx

35c9de3

Test 3.13 with numpy 1.26.0

hschreiber reviewed Feb 12, 2025

View reviewed changes

VincentRouvreau added 5 commits February 14, 2025 13:42

code review: Modify Alpha complex utils to keep backward compatibilit…

44a6747

…y but to fit output_squared_filtration feature

code review: forgotten modif

4227322

code review: Modify Delaunay Cech complex utils to keep backward comp…

404cb39

…atibility but to fit output_squared_filtration feature

code review: specify sqrt in SimplicialComplexForAlpha concept

021561a

code review: Bad log in ut

a8d8634

VincentRouvreau added the enhancement New feature or request label Feb 14, 2025

VincentRouvreau added 2 commits February 17, 2025 10:27

Merge branch 'delaunay_square_root_filtrations' into delaunay_square_…

f33ad10

…root_filtrations_assert_weighted

code review: output_squared_values should always be true in the weigh…

4ee4878

…ted case + some tests

hschreiber reviewed Feb 17, 2025

View reviewed changes

code review: proposal from Hannah

bc7fbb6

hschreiber approved these changes Feb 18, 2025

View reviewed changes

VincentRouvreau added the 3.11.0 GUDHI version 3.11.0 label Feb 18, 2025

VincentRouvreau merged commit 1f274c8 into GUDHI:master Feb 18, 2025
7 checks passed

VincentRouvreau deleted the delaunay_square_root_filtrations branch February 18, 2025 14:32

VincentRouvreau mentioned this pull request Feb 18, 2025

Filtration values of the alpha complex (squared radius) #771

Closed

mglisse reviewed Feb 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

output squared filtration values true/false mechanism for Delaunay complexes #1172

output squared filtration values true/false mechanism for Delaunay complexes #1172

VincentRouvreau commented Feb 10, 2025 •

edited

Loading

hschreiber Feb 12, 2025

VincentRouvreau Feb 14, 2025

hschreiber Feb 14, 2025

VincentRouvreau Feb 14, 2025

hschreiber Feb 12, 2025

VincentRouvreau Feb 14, 2025 •

edited

Loading

hschreiber Feb 12, 2025

VincentRouvreau Feb 13, 2025

hschreiber Feb 13, 2025

VincentRouvreau Feb 14, 2025 •

edited

Loading

hschreiber Feb 12, 2025

VincentRouvreau Feb 14, 2025

VincentRouvreau commented Feb 14, 2025

VincentRouvreau commented Feb 17, 2025

hschreiber Feb 17, 2025

VincentRouvreau Feb 17, 2025

mglisse commented Feb 19, 2025

mglisse Feb 19, 2025

hschreiber commented Feb 19, 2025 •

edited

Loading

		* \note Weighted Alpha complex can have negative filtration values. If `output_squared_values` is set to `false`,
		* filtration values will be `NaN` in this case.

		// For users to be able to define their own sqrt function on their desired Filtration_value type
		using std::sqrt;

		* \exception std::invalid_argument In case of a Weighted Alpha complex with `Output_squared_values` set to `false`,
		* as Weighted Alpha complex can have negative filtration values.

output squared filtration values true/false mechanism for Delaunay complexes #1172

output squared filtration values true/false mechanism for Delaunay complexes #1172

Conversation

VincentRouvreau commented Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau commented Feb 14, 2025

VincentRouvreau commented Feb 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mglisse commented Feb 19, 2025

Choose a reason for hiding this comment

hschreiber commented Feb 19, 2025 • edited Loading

VincentRouvreau commented Feb 10, 2025 •

edited

Loading

VincentRouvreau Feb 14, 2025 •

edited

Loading

VincentRouvreau Feb 14, 2025 •

edited

Loading

hschreiber commented Feb 19, 2025 •

edited

Loading