Restructure ripser and all language bindings

[scikit-tda]

Restructure ripser and all language bindings

[ctralie]

I like the idea of separating out the C++ code and keeping it as a fork of ripser, but honestly I would need to know more about the "C bindings" by @mtsch before I could be sure how I feel about it. What are the planned features there?
Thanks,
Chris

[mtsch]

Hello,

Sorry for taking so long, I was pretty busy and then I went away for a week. I had the chance to take a look at the code and from what I can see, the pythondm function is pretty much what I had in mind.
The only thing I'm not so sure about is storing lengths and indices as floats, but I don't really see a better way to do it.

I just need to add a function that transforms it into a C array and declare it as extern "C".

Cheers, Matija

[ctralie]

Hello Matija, I am so glad this saved you the duplicate effort! I suspected this might be the case. I agree that I have been a bit fast and loose with types. There is probably a way to do it better by returning a Cython object that encapsulates the proper types. But this was a quick and dirty solution that works for any point cloud that would be computable in ripser at the moment anyway (even getting up to 2000 points, things start to slow down). That said, if you have any ideas, please let me know. By the way, there is a new, more elegant version of the wrappers that I'm working on: https://github.com/ctralie/ripser/tree/sparsedm The branch is just about done, and it even has sparse matrix support! But I appear to have broken representative cocycles. So I'm working that out. Finally, with this new branch, things are tidy enough that I think it would be pretty easy to basically duplicate pythondm to Javascript and Julia entry points, keeping the rest of the code in tact. These functions are relatively short, so simply concatenating those entry points could be a good way to go (though it is beyond the scope of this explicitly python library, we could have this be a staging point for multiple languages). Thanks, Chris

…

On Tue, May 8, 2018, 8:01 AM mtsch ***@***.***> wrote: Hello, Sorry for taking so long, I was pretty busy and then I went away for a week. I had the chance to take a look at the code and from what I can see, the pythondm function is pretty much what I had in mind. The only thing I'm not so sure about is storing lengths and indices as floats, but I don't really see a better way to do it. I just need to add a function that transforms it into a C array and declare it as extern "C". Cheers, Matija — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABNh5iY6kBjbXP3ZjCVlqC_9Fr3Pkn8Qks5twYkJgaJpZM4TggTY> .

[mtsch]

I added the C thing for both the regular and sparse versions in #21.
About the outputs: do you maybe think it would be a good idea to return 4 arrays, one for the numbers of intervals in each dimension, one for the births and deaths, one for cocycle lengths and one for the cocycles. The C interface would then be something like int cripser(int** numperclass, float** birthsanddeaths, int** cocyclelength, int** cocycles, ...). The function would return the number of dimensions.
I'm not sure what the best way to do this is in C++ or how that works with Python, since I don't have a lot of experience with those.

[ctralie]

Okay great! I merged that in, thanks.

One warning, though. I indeed got sparse matrices to work on that branch, but I seem to have broken cocycles. So I'm still working that out, which is why I haven't merged it to master yet. Luckily, this is something internal, and it shouldn't affect any of the code you've written. But careful using it until that's resolved.

Let me reach out to Uli Bauer again to see if he can help me fix the cocycles

[mtsch]

No worries about the cocycles, I haven't needed them yet and they were never available in my wrapper. I do plan on supporting them, but I have some major restructuring to do first.

By the way, huge thanks for doing this, it's gonna be really useful for me!

[ctralie]

@mtsch @sauln should I close this issue?

Or should we continue the discussion? I think the code is setup now that very small entry point blocks can be added to support other languages. Not sure if you want to keep my fork as just a Python fork or have those other languages in the fork as well?

[mtsch]

I think it's better to have the bindings for different languages separated, or it could get messy. Either way, the Julia bindings need to be separate because Julia's package manager downloads code from Github directly and it requires a certain repo structure.
It would be perfect if the C++ code was separated from the bindings (or maybe merged into the main repo if Uli would have it), but I can live with the current solution as well. I would however propose a libripser branch that only includes the C++ part and a Makefile that builds a .so/.dll file. I can prepare a PR this week if you agree. Another nitpick is that we should probably rename the pythondm function to something else because it's not really Python specific.

Beside all that, It's looking great!

[ctralie]

Yes, now that I see how it is being used I completely agree with the all of the above. Let's keep the C++ branch with my fork now to avoid driving Uli nuts. Feel free to make a pull request this week. There's one more small thing I want to do with the C++ code still (issue #25), but I can just merge that in if you get to your pr before me. Once that's done I'm hoping the C++ code will be relatively frozen for some time, modulo whatever bugs need to be fixed Thanks, Chris

…

On Sat, May 12, 2018, 2:47 PM mtsch ***@***.***> wrote: I think it's better to have the bindings for different languages separated, or it could get messy. Either way, the Julia bindings need to be separate because Julia's package manager downloads code from Github directly and it requires a certain repo structure. It would be perfect if the C++ code was separated from the bindings (or maybe merged into the main repo if Uli would have it), but I can live with the current solution as well. I would however propose a libripser branch that only includes the C++ part and a Makefile that builds a .so/.dll file. I can prepare a PR this week if you agree. Another nitpick is that we should probably rename the pythondm function to something else because it's not really Python specific. Beside all that, It's looking great! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABNh5kExfK1mtd65lczIen8babVT5FgFks5txy5EgaJpZM4TggTY> .

[ctralie]

Hi @mtsch, I'm going to try to resolve #25 in the next few days. Once I do that, the C++ code should be frozen for a while. That would be a good time to make a pull request.
Thanks,
Chris

[ctralie]

@mtsch, I merged in the new cython wrappers to master because I need to make another branch for matlab that uses then. Could you make a pull request with the stuff you need?
Thanks,
Chris