Output floating point types about giotto-ph HOT 7 CLOSED

Good remark.

By default, numpy uses a 64-bit precision for float representation.
We need to currently cast input matrices to 32-bit, because the C++ core currently only supports 32-bit floating point representation.
For the output, If I am not mistaken, this line cast implicitly from 32-bit to 64-bit for float representation in the Python interface.
The cast from 32-bit to 64-bit should be safe in terms of precision.

I don't think it is a bad idea to return to users the default type for numpy.float from the Python interface. But, I do not have a strong reason to not return 32-bit as how it is currently returned from the C++ core.

from giotto-ph.

I don't think it is a bad idea to return to users the default type for numpy.float from the Python interface. But, I do not have a strong reason to not return 32-bit as how it is currently returned from the C++ core.

The reason I think it is not great to upcast downcasted floats is that users who pass distance matrices (as 64-bit arrays) might decide to "match" distance matrix values with the output bars. If 64-bit, they would be well-justified to expect that every birth and death corresponds exactly to an entry in the distance matrix. If 32-bit, they would at least have an indirect hint as to what might have happened.

Incidentally, this is related to another issue, namely that a possible enhancement would be to add a 64-bit implementation and allow users to pass a precision kwarg (default: single, but with the possibility of passing double).

from giotto-ph.

The reason I think it is not great to upcast downcasted floats is that users who pass distance matrices (as 64-bit arrays) might decide to "match" distance matrix values with the output bars. If 64-bit, they would be well-justified to expect that every birth and death corresponds exactly to an entry in the distance matrix. If 32-bit, they would at least have an indirect hint as to what might have happened.

Good argument about trying to match the output with the input ! Definitely, if they try to match and because we outputs 64-bit float, they would expect to find the exact same values they used as input. But, because of the downcast, this values are slightly different.

Incidentally, this is related to another issue, namely that a possible enhancement would be to add a 64-bit implementation and allow users to pass a precision kwarg (default: single, but with the possibility of passing double).

This shall not be in my opinion too much work, it is adding a template argument to the ripser function in the C++. Currently I am on a different project, but if this should be a must to have, then I can see to do it earlier.

from giotto-ph.

if this should be a must to have, then I can see to do it earlier.

I think there are other features that would be cooler to have first, but they are harder (cocycles, persistence pairs, isolate edge collapser). Maybe we should make a kanban for these? Anyway, if it is really that simple, why not add it I say.

from giotto-ph.

As always, it seems simple, maybe some issues will be encountered ...
But even if it seems easy, we should add corresponding test in order to ensure that what we add works when expected.
This alone takes some time.

Maybe we should make a kanban for these?

Definitely, but if possible, if we can centralize this in Github directly, it would be really cool !

from giotto-ph.

Definitely, but if possible, if we can centralize this in Github directly, it would be really cool !

Done :)

from giotto-ph.

Nice, thank you !

from giotto-ph.