Skip to content
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

Not reproducible #34

Closed
skygering opened this issue Mar 7, 2023 · 14 comments
Closed

Not reproducible #34

skygering opened this issue Mar 7, 2023 · 14 comments

Comments

@skygering
Copy link
Contributor

skygering commented Mar 7, 2023

If I run the example in the documentation twice, even using the exact same points, I get very very slightly different answers on some of the points.

For example, if I run:

using VoronoiCells
using GeometryBasics
using Random

rect = Rectangle(Point2(0, 0), Point2(1, 1))
rng = Random.MersenneTwister(1337)
points = [Point2(rand(rng), rand(rng)) for _ in 1:10]


cells1 = voronoicells(points, rect).Cells
cells2 = voronoicells(points, rect).Cells

same = true
for i in eachindex(cells1)
    same = same && all(cells1[i] .== cells2[i])
end

I consistently get different values on at least some of the points and same is false. Granted, these are small differences, e.g. 0.5006658246439755 vs. 0.5006658246439761, but given I have to scale the results quite a lot to get a tessellation of my needed size, these can become quite large. It also prevents my simulation from being reproducible.

Do you have any idea where this might come from? I am willing to take a look as well!

Thanks!

@robertdj
Copy link
Collaborator

Well spotted and thanks for reporting it here.

I use the VoronoiDelaunay package to do the actual work of computing the points in the tessellation -- VoronoiCells just capture the results in a convenient manner.

It would be great if you can take a look. I'm quite out of sync with the inner workings of VoronoiDelaunay, but I suppose a starting point could be to check my claim about which package introduce the "randomness".

@skygering
Copy link
Contributor Author

Thank you for responding! I will work on this when I have some time over the next few weeks. I will update if I have any progress.

@skygering
Copy link
Contributor Author

I think that I found the issue. Adding in an optional RNG parameter to the call to pull! in VoronoiCells, which calls shuffle! in VoronoiDelaunay seems to do it. I have opened a pull request to VoronoiDelaunay and will make a pull request here as well if that goes through!

@skygering
Copy link
Contributor Author

@robertdj I see that you're also a contributor on VoronoiDelaunay. Do you think you could take a look at my pull request? It is 3 lines and fully backwards compatible. That will allow me to make my pull request here (I have already written tests and everything) so that the answers won't be non-deterministic anymore. Thank you so much!

@robertdj
Copy link
Collaborator

robertdj commented Apr 2, 2023

Hi @skygering Sorry for being so slow! I have some time off and is much more responsive the next week.

Great that you found the issue! It's true that I have contributed to VD, but it is a long time ago and I don't think I have the street credit to approve PRs anymore.

But I'll try harder to at least answer here when you post and I'm happy to accept well tested changes :-)

@skygering
Copy link
Contributor Author

@robertdj Would you have any idea who to contact regarding the PR on VD? It doesn't seem like anyone has worked on the repo for 9 months. This is holding up my progress, so I would like to get it resolved if possible.

Thanks!

@robertdj
Copy link
Collaborator

robertdj commented Apr 4, 2023

No, sorry. The latest issue also suggests that the repo is unmaintained :-(

If it's only for personal use maybe you can fork it?

@skygering
Copy link
Contributor Author

It is eventually for public use, so I would really rather not make a copy of a perfectly good piece of software to change 3 lines, one of which is an import 😆 I will ping the geometry channels in the Julia slack and see what can be done

@skygering
Copy link
Contributor Author

@robertdj Good news! I got my PR merged into VoronoiDelaunay, so I was able to open my pull request here. Let me know if there is anything I can do 😄

@robertdj
Copy link
Collaborator

robertdj commented Apr 6, 2023

Great! I'll have a look.

@robertdj
Copy link
Collaborator

robertdj commented Apr 6, 2023

It's merged 🎉 What would be an appropriate version for this change?

@skygering
Copy link
Contributor Author

skygering commented Apr 7, 2023

Thank you! Can we just bump the patch version? It is backwards compatible so it shouldn't be breaking, so just v0.3.1? Although it does require a higher version of VoronoiDelaunay. Thoughts?

@robertdj
Copy link
Collaborator

robertdj commented Apr 8, 2023

Hmm. True, but it also adds a new feature. So I'm leaning more towards bumping the minor to 0.4.0

@skygering
Copy link
Contributor Author

skygering commented Apr 10, 2023

@robertdj That would be fantastic! Thanks for getting it merged and tagged.

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

No branches or pull requests

2 participants