search

giotto-ai / pyflagser

Python bindings and API for the flagser C++ library (https://github.com/luetge/flagser).
Other
13 stars 14 forks source link

Enable in memory option in C++ backend #67

Open reds-heig opened 3 years ago

reds-heig commented 3 years ago

Reference Issues/PRs

61

What does this implement/fix? Explain your changes.

This PR integrates latest changes from flagser backend. In which the option in_memory is now available when computing homology. This options allows a tradeoff of using more memory in order to speed up computation.

Any other comments?

In order to support the in_memory data structure, I needed to duplicate persistence_computer_t part that manages the part to retrieve results. In my opinion it's not the best way of doing this, but due to how retrieving values from results was implemented in flagser I cannot see how to do differently. If anyone has a better solution/approach, please let me know !

Quoting the author:

The main difference between memory and non-memory is that you can have an efficient lookup in the data structure instead of relying on hashing for looking up indices of simplices. Depending on the use case, this can have huge performance differences, either in terms of compute time or memory, so choosing the appropriate version is important.

MonkeyBreaker commented 3 years ago

BTW, I didn't add test for this one. But I wasn't sure about the best way of adding them. in_memory is now an option when calling flagser_unweighted or flagser_weighted, meaning that we could reuse the same test and expect exact same results. @ulupo do you have an idea on how to reuse the same test with a new parameter ? (I don't think it's necessary to run the heavy test on this option)

ulupo commented 2 years ago

@MonkeyBreaker should we also go ahead with this PR?

MonkeyBreaker commented 2 years ago

I think that I should add a test, something simple. Compute Flagser on the same data, and expect same results. After that we can merge this, I will do it this week-end if it works for you.

MonkeyBreaker commented 1 year ago

After we merge new github actions, I will add the missing test and hope that all test passes. Should I also add a table showing examples of run with option enable/disable reporting runtime and memory consumption ? @ulupo what do you think ?

ulupo commented 1 year ago

Thanks @MonkeyBreaker!

Should I also add a table showing examples of run with option enable/disable reporting runtime and memory consumption ? @ulupo what do you think ?

I think that some draft version of this would be very helpful, but let's not spend too much time on this!

ulupo commented 1 year ago

This feature should be the main highlight of v0.4.6 :)