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 flagser_count capabilities from C++ flagser and have two modules to handle coeff = or != 2 #37

Closed gtauzin closed 4 years ago

gtauzin commented 4 years ago

What does this implement/fix? Explain your changes.

Due to high demand, pyflagser is enabling the direct use of count_cells functionalities from C++ flagser.

Additionally:

Any other comments?

This adds the pybind11 bindings, the python API and the appropriate tests.

gtauzin commented 4 years ago

The tests are failing as, for two example files, there is a discrepency between the cell counts as outputed by flagser and flagser-count. As this is a problem I experience running C++ flagser as well, I have opened an issue on the flagser GitHub.

gtauzin commented 4 years ago

@ulupo My last commit adds a reshape to the dgms outputed by flagser_weighted. It is necessary for FlagserPersistence to deal with empty subdiagrams. It is also consistent with what we documented it should return, so I put it here.

ulupo commented 4 years ago

@gtauzin thanks for the explanation, I was just about to ask about it!

ulupo commented 4 years ago

@gtauzin thanks for the reminder. I'll find time for this over the weekend.

ulupo commented 4 years ago

@gtauzin just reiterating the general question in my last review ("why doesn't flagser_weighted get the coefficient treatment in a similar way as flagser_unweighted?") in case it slipped.

gtauzin commented 4 years ago

@gtauzin just reiterating the general question in my last review ("why doesn't flagser_weighted get the coefficient treatment in a similar way as flagser_unweighted?") in case it slipped.

I missed it, sorry. It's fixed as there is no reason, I just forgot.

MonkeyBreaker commented 4 years ago

General question: why doesn't flagser_weighted get the coefficient treatment in a similar way as flagser_unweighted?

Generally LGTM, just left a couple of comments. The C++/pybind11 part is slightly above my pay grade, I wonder if @MonkeyBreaker could give it a quick look?

Hi, sorry for my late reply, I can give a review this afternoon if it's fine for both of you :)

ulupo commented 4 years ago

@MonkeyBreaker yes please :)

MonkeyBreaker commented 4 years ago

LGTM !

Changes are good for me :)