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.

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

Multiparameter simplextree, C++ part. #976

Open
wants to merge 123 commits into
base: master
Choose a base branch
from

Conversation

DavidLapous
Copy link
Member

Cpp part of PR #817.
List of changes :

  • new is_multi_parameter flag in the simplextree's options to deal with filtration values that behave as array-like instead of float-like.
  • Filtration values are now as const ref to prevent array-like copies
    • This implies adding static members for trivial filtrations : null_value and inf_
  • new member filtration_mutable to have a non-const access to the filtration.
  • For multiparameter filtrations, deactivated the assign behavior of the insert to prevent unwanted behaviors (and stay 1-critical). The filtration property is no longer maintained.
  • New array-like structure as Filtration_value, child of std::vector with numpy-like pointwise operations, in Simplex_tree/multi_filtrations/Finitely_critical_multi_filtrations.h
  • The Simplex_tree/Simplex_tree_multi.h contains the options of a simplextree multi, functions that can convert a 1-parameter simplextree to a multiparameter simplextree and vice versa, aswell as useful functions to push filtration values on a discrete grid (for discrete computations.
  • I also copied the unit tests of the simplextree to apply them to this new structure.

Feel free to move files/functions if they don't follow Gudhi's standard, or commit if its moral changes.

private:
Filtration_value filt_;
};
struct Filtration_simplex_base_dummy {
Filtration_simplex_base_dummy() {}
void assign_filtration(Filtration_value GUDHI_CHECK_code(f)) { GUDHI_CHECK(f == 0, "filtration value specified for a complex that does not store them"); }
Filtration_value filtration() const { return 0; }
const Filtration_value& filtration() const { return null_value; }
static constexpr Filtration_value null_value={};
Copy link
Contributor

@VincentRouvreau VincentRouvreau Sep 29, 2023

Choose a reason for hiding this comment

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

If we say:

struct MyOptions : Simplex_tree_options_multidimensional_filtration {
  // is_multi_parameter but not store_filtration
  static const bool store_filtration = false;
};

Simplex_tree<MyOptions> stree;

We are facing the error:

gudhi-devel/src/Simplex_tree/include/gudhi/Simplex_tree.h:165:39: error: the type 'const Filtration_value' {aka 'const Gudhi::multiparameter::multi_filtrations::Finitely_critical_multi_filtration<float>'} of 'constexpr' variable 'Gudhi::Simplex_tree<MyOptions>::Filtration_simplex_base_dummy::null_value' is not literal
     static constexpr Filtration_value null_value={};
                                       ^~~~~~~~~~

But maybe this error is normal ? I was wondering to see if we should static_assert some error in this weird case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A static_assert could be a good idea, as the multi parameter tree only differentiate it self from the standard with its filtration values. If store_filtration == false, is_multi_parameter set to true makes no sense. But the problem could also be solved by removing null_value (see Issue #977), then we can just document that is_multi_parameter is ignored if store_filtration == false.

Copy link
Member Author

@DavidLapous DavidLapous Oct 3, 2023

Choose a reason for hiding this comment

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

A simple fix is to do a static_assert(!store_filtration || is_multi_parameter). thoughts ?

Copy link
Member

Choose a reason for hiding this comment

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

I guess you meant static_assert(store_filtration || !is_multi_parameter) instead?

Comment on lines +609 to +620
static const Filtration_value& filtration(Simplex_handle sh){
if (sh != null_simplex()) {
return sh->second.filtration();
} else {
return std::numeric_limits<Filtration_value>::infinity();
return inf_;
}
}
static Filtration_value& filtration_mutable(Simplex_handle sh){
if (sh != null_simplex()) {
return sh->second.filtration();
} else {
return inf_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Already from the previous PR:

I am wondering if we could not just keep the version with the non const, as the validity of the filtration in the simplex tree is already responsibility of the user. This would avoid having to find a different name when it does exactly the same thing than filtration(). Or could there be performance issues when using non const references for numerical values (i.e., when the simplex tree is not multi)? If it is the case, an ugly way to solve this would be to return a std::conditional to switch between Filtration_value& and Filtration_value depending on the value of is_multi_parameter.

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 don't have a fixed point of view on this. So its only a matter of what's the gudhi style of accessing a filtration value by reference.

Copy link
Member

Choose a reason for hiding this comment

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

If they really do the same thing, I am ok with filtration returning a non-const reference. But I wonder if filtration_mutable actually needs to return infinity for the null simplex. If we decided instead that filtration_mutable only accepts real simplices (GUDHI_CHECK that sh!=null_simplex()), it could make sense for it to be a separate function, and it would let us declare inf_ const.
IIRC (that was a while ago, so I am likely wrong), handling null_simplex was mostly for convenience for the output of persistent cohomology, not some fundamental reason.

* Modification(s):
* - YYYY/MM Author: Description of the modification
*/
#ifndef SIMPLEX_TREE_MULTI_H_
Copy link
Collaborator

Choose a reason for hiding this comment

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

Simplex_tree_multi_utilities.h seems more precise and less misleading to me than Simplex_tree_multi.h. What do you think?

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'm not a big fan of the utilities name, but the name can be changed indeed. Let's fix the other issues and we'll decide that once the content of this file is fixed.

std::vector<int> simplex;
for (auto vertex : st_multi.simplex_vertex_range(simplex_handle))
simplex.push_back(vertex);
typename simplextree_multi::Options::value_type f = dimension >= 0 ? st_multi.filtration(simplex_handle)[dimension] : 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the use of dimension to possibly be negative?

Copy link
Member Author

Choose a reason for hiding this comment

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

put every filtration values of st to 0. May be useful to copy a simplextree when we don't care about filtration values.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I see. You should say this in the description of dimension, now that you added the doc.




// Turns a multi-parameter simplextree into a 1-parameter simplextree
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should precise that the filtration values are not token over (which makes sense, but still). Could also add, that for the filtration values, linear_projection can be used.

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'm not sure what you mean, each flatten function take into account filtration values.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh? You are right, I think I completely missed a line here as I had the feeling that the filtration value was always put to 0. Which would also explain why I didn't understood the role of the dimension.

Filtration_simplex_base_real() : filt_{} {}
void assign_filtration(const Filtration_value& f) { filt_ = f; }
const Filtration_value& filtration() const { return filt_; }
Filtration_value& filtration() { return filt_; }
Copy link
Contributor

Choose a reason for hiding this comment

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

TBD, but I am not sure we should add this method.
And if we add it we should add it to the dummy one.

*
* @param n Number of parameters.
*/
One_critical_filtration(int n) : Base(n, -T_inf) {};
Copy link
Member

Choose a reason for hiding this comment

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

This (and some others below?) should probably be marked explicit.
Would it cause problems to merge it with the next constructor?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants