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

Enable in memory option in C++ backend #67

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

Conversation

reds-heig
Copy link
Collaborator

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.

julian added 3 commits March 30, 2021 10:58
Some hacks needed to be done to support two different outputs due to templating

Signed-off-by: julian <[email protected]>
@reds-heig reds-heig requested a review from ulupo March 30, 2021 09:12
@MonkeyBreaker
Copy link
Collaborator

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)

Copy link
Contributor

@ulupo ulupo left a comment

Choose a reason for hiding this comment

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

Thanks for this, great work! For now, just some wording suggestions. I'll get back to you about tests shortly.

pyflagser/flagser.py Outdated Show resolved Hide resolved
pyflagser/flagser.py Show resolved Hide resolved
MonkeyBreaker and others added 2 commits April 26, 2021 10:53
@giotto-ai giotto-ai deleted a comment from azure-pipelines bot Apr 26, 2021
@ulupo
Copy link
Contributor

ulupo commented Dec 7, 2021

@MonkeyBreaker should we also go ahead with this PR?

@MonkeyBreaker
Copy link
Collaborator

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
Copy link
Collaborator

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 ?

@MonkeyBreaker MonkeyBreaker linked an issue Aug 16, 2022 that may be closed by this pull request
@ulupo
Copy link
Contributor

ulupo commented Aug 25, 2022

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
Copy link
Contributor

ulupo commented Aug 25, 2022

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

@giotto-ai giotto-ai deleted a comment from azure-pipelines bot Aug 25, 2022
@giotto-ai giotto-ai deleted a comment from azure-pipelines bot Aug 25, 2022
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.

flagser_memory support
3 participants