-
Notifications
You must be signed in to change notification settings - Fork 60
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
Restructure ripser and all language bindings #10
Comments
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? |
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 I just need to add a function that transforms it into a C array and declare it as Cheers, Matija |
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>
.
|
I added the C thing for both the regular and sparse versions in #21. |
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 |
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! |
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. Beside all that, It's looking great! |
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>
.
|
@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? |
This is a continuation of a conversation from mtsch/Ripser.jl#1
The current ripser+bindings ecosystem is a bit of a mess. We currently have multiple repositories and forks, all accomplishing different goals. I suggest we restructure the repos so that the ecosystem can grow smoothly. This current fork has diverged greatly from the original repository and I don't foresee us ever merging it back.
There is also intention to create Matlab bindings and to port the old Wasm bindings. @mtsch is working on C bindings that might simplify this whole process and enable many other languages.
There are probably a couple different ways we could do this. One that stands out to me would be to split this repo into 2, one with the modified cpp and the other with the Python bindings. Then all bindings repositories could point towards the cpp library. Only the cpp repo would be a fork of ripser, the rest would be standalone repos.
Ideally, we could put all repos under the @Ripser organization, but @ubauer would have to give everyone access to the organization.
@ctralie, @mtsch, what do you two think?
The text was updated successfully, but these errors were encountered: