Proposal to deal with explicit infinite edges #53
Conversation
Addresses issue giotto-ai#37
| @@ -1564,11 +1587,9 @@ class ripser | |||
| std::greater<>()); | |||
| #endif | |||
| for (size_t i = 0; i < idx_essential; ++i) { | |||
| if (!std::isinf(essential_pair[i])) { | |||
There was a problem hiding this comment.
This is about ensuring that births are not infinite ("the bar is never born"), but in this proposal it is not needed because is_born makes sure we don't end up in this case.
|
One concern about this PR or similar approaches is that the additional control flow might introduce performance regressions, so I would suggest running extensive profiling before merging if we decide to address #37 at all. |
My intuition after looking at the changes is that the overhead will be not be noticed. A benchmark will confirm this, but I am pretty sure we won't notice a slowdown. |
Great! |
|
|
|
@ulupo after resolving merging conflicts with latest master I just performed a benchmark:
It seems that the runtime impact is not relevant. All the benchmark were done with |
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
|
@MonkeyBreaker fantastic about the runtimes! |
Signed-off-by: julian <monkeybreaker@protonmail.com>
Much easier to write :) Signed-off-by: julian <monkeybreaker@protonmail.com>
Signed-off-by: julian <monkeybreaker@protonmail.com>
Signed-off-by: julian <monkeybreaker@protonmail.com>
|
@ulupo thank you for this PR and exploring cases where we had some issues ! I will proceed with the Merge of this PR, thank you again for your great work :) |
Just a way to start the discussion on whether and how to address #37. I believe this proposal has at least the correct logic, so could serve as an initial reference.