Skip to content
This repository has been archived by the owner on Oct 23, 2019. It is now read-only.

MiniballNext #28

Closed
jw3126 opened this issue Jun 26, 2018 · 25 comments · Fixed by #30
Closed

MiniballNext #28

jw3126 opened this issue Jun 26, 2018 · 25 comments · Fixed by #30

Comments

@jw3126
Copy link
Collaborator

jw3126 commented Jun 26, 2018

I created the MiniBallNext repository. It is a complete rewrite from scratch of Miniball. Differences are:

  • Its implemented based on Gaertners paper, not the C++ implementation. As such I put an MIT license on it.
  • The api is different. The input is an iterator of points, the output is a pair (center, radius).
  • It is very modular
  • In my opinion the implementation is very readable and close to the math, but I am biased here 😄

It is more numerically stable than current Miniball.jl master, not sure if it is as good as Gaertners latest C++ code. While MiniBallNext is not perfect, I think it is a better foundation for future improvements then current Miniball.jl master. Please take a look and give me feedback @ahojukka5 @ovainola @TeroFrondelius
If you like it, I will provide a PR.

@ahojukka5
Copy link
Owner

Some of the files are missing license header and also some docstrings. But in general, this looks promising approach for developing new algorithms.

But I would suggest that we are not branching the develop for several different projects. I think one miniball is enough for one programming language. So how about if we (somehow) combine these two packages into a single one?

@jw3126
Copy link
Collaborator Author

jw3126 commented Jun 28, 2018

Yeah MiniBallNext should be a temporary thing. My hope was to provide a PR here, that replaces the content of Miniball.jl by the content of the MiniBallNext.jl repository.
Of course only if you guys are fine with this idea and I will add JuliaFEM to the license.
Sorry if this was unclear from my first post.

@jw3126
Copy link
Collaborator Author

jw3126 commented Jul 5, 2018

@ahojukka5 @ovainola @TeroFrondelius bump. Are you open to having the content of Miniball.jl replaced by the content of MiniBallNext.jl or not? Any specific parts about MiniBallNext that you want / don't want to have here?

@TeroFrondelius
Copy link
Contributor

Yes I am open to everything.

@ahojukka5
Copy link
Owner

My suggestion:

Let's create a submodule / directory Old or similar and move src.jl to there with GPL license. Then user can type using Miniball.Old to use the old version. Then we replace the content of the package with a new code and re/dual license package. Or is this too complicated approach regarding to licensing?

@jw3126
Copy link
Collaborator Author

jw3126 commented Jul 5, 2018

I think it sounds good. I think we can put MIT on the whole package and add a sentence that it contains code based on Gaertners Cxx implementation. A bit similar to what is done here: https://github.com/JuliaLang/julia/blob/master/LICENSE.md. In addition we should put LGPL headers into the old files of course. We could also dual license it under MIT/LGPL, though I guess MIT allows you to do anything that LGPL does anyway.

@ahojukka5
Copy link
Owner

After thinking this a little bit longer I think we should not replace the content of this package with a new code. Someone might get angry after doing Pkg.update to find out that the license has been changed and the code has been totally replaced with a new one.

How about if we figure out a new name for a new implementation and keep this old one how it is now. We can then write to the readme file of this package that there exists a new implementation programmed more "Julia style" and development continues there.

@jw3126
Copy link
Collaborator Author

jw3126 commented Jul 6, 2018

Okay sounds fine to me. The hard part will be finding a name that is as good as Miniball.jl 😄 Some ideas:

  • SmallestEnclosingSpheres.jl a bit long, but easy to google
  • SmallestEnclosingBalls.jl same
  • MiniBall.jl too close to Miniball.jl
  • MiniSphere.jl

@TeroFrondelius
Copy link
Contributor

I like SmallestEnclosingSphere.jl I think it is the most Julian style (although with Sphere not with Spheres), few more options:

  • MinimumBall.jl
  • SmallBall.jl
  • SmallestSphere.jl

If you want, we can take the new repo under JuliaFEM, of course there might be better suiting organisations as well.

@ahojukka5
Copy link
Owner

@jw3126
Copy link
Collaborator Author

jw3126 commented Jul 6, 2018

I think we have a couple of good names! My favorite is BoundingSphere.jl. It is descriptive and not too long. SmallBall.jl is also a cool name, but harder to discover I think.

@TeroFrondelius
Copy link
Contributor

+1 for BoundingSphere.jl

@jw3126
Copy link
Collaborator Author

jw3126 commented Jul 9, 2018

Then lets go with BoundingSphere.jl? @ovainola are you fine with this, too?

@ovainola
Copy link
Contributor

ovainola commented Jul 10, 2018

Sure, naming is fine. @ahojukka5 I'd suggest that the old code could also be in a different branch (f.ex. "old" which would be with the original code). In that way there wouldn't be two separate license files in the same branch. Also updating separate branches would be clearer (commit history wouldn't consist of fixes to new code and the old code).

The old branch could also be called "legacy"

@jw3126
Copy link
Collaborator Author

jw3126 commented Jul 10, 2018

I really like @ahojukka5 suggestion to start with a fresh BoundingSphere.jl repo, instead of mixing it with the current Miniball.jl. It is a clean solution, we don't have to think about licenses and we can't accidentally break users code. @ovainola I don't see an advantage of having the old code and the new code in the same repo, even on different branches.

@ovainola
Copy link
Contributor

@jw3126 it doesn't give any advantage, there was just an comment on storing the old codes so just commenting on that. If new code is far better, there is no reason to store the old one

@jw3126
Copy link
Collaborator Author

jw3126 commented Jul 11, 2018

Of course the old code is still stored, just not in the same repo as the new code. This will also make it easier to compare stability/performance/accuracy etc of old and new code. Running code from two branches is always a bit painful.
Anyway I think the next step would be that someone creates a BoundingSphere.jl repo under the JuliaFEM organization and gives me push access. Then I can move the MiniballNext code (optionally also a Miniball.jl branch) there and we can make it compliant with JuliaFEM coding standards (e.g. license headers, .travis etc...).

@ahojukka5
Copy link
Owner

I think we already made conclusions how to proceed with this.

@TeroFrondelius
Copy link
Contributor

I propose that we will rename MiniBallNext.jl to BoundingSphere.jl and move it to JuliaFEM organisation. @jw3126 you need to give me or Jukka admin rights to the repo and either of us can move it.

@jw3126
Copy link
Collaborator Author

jw3126 commented Jul 11, 2018

Ok sounds fine, I invited both of you.

@TeroFrondelius
Copy link
Contributor

TeroFrondelius commented Jul 11, 2018

@jw3126 I will need admin rights to access settings menu, where I can perform the Transfer ownership to the JuliaFEM organisation.

@jw3126
Copy link
Collaborator Author

jw3126 commented Jul 11, 2018

I don't know how to give you admin access. Maybe thats only possible for repos owned by organizations, I am not sure. Anyway I transfered ownership to you @TeroFrondelius (I cannot transfer to JuliaFEM directly myself).

@TeroFrondelius
Copy link
Contributor

Transfer and renaming done. https://github.com/JuliaFEM/BoundingSphere.jl

@TeroFrondelius
Copy link
Contributor

I also added this text to the README:

This repo has some nasty bug, see broken tests. After bug hunt a decission to refactor led to the rewriting of the package, which gave the opportunity to change the license. Development will continue in https://github.com/JuliaFEM/BoundingSphere.jl check if realesed version exists and move using the BoundingSphere.jl instead.

@TeroFrondelius
Copy link
Contributor

I think we should follow these instructions: https://discourse.julialang.org/t/how-do-i-deprecate-a-package/14129

What do you think?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants