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

Voronoi #30 #46

Merged
merged 3 commits into from
Mar 2, 2021
Merged

Voronoi #30 #46

merged 3 commits into from
Mar 2, 2021

Conversation

virgile-baudrot
Copy link
Contributor

Add Discrete Voronoi generation using NearestNeighborElement #30

Type of change
Please indicate the relevant option(s)

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ❇️ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📖 This change requires a documentation update

Checklist

  • The changes are documented
    • The docstrings of the different functions describe the arguments, purpose, and behavior
    • There are examples in the documentation website
  • The changes are tested
  • The changes do not modify the behavior of the previously existing functions
    • If they do, please explain why and how in the introduction paragraph
  • For new contributors - my name and information are added to .zenodo.json
  • The Project.toml field version has been updated
    • Change the last number for a v0.0.x series release, a bug fix, or a performance improvement
    • Change the middle number for additional features that do not break the current test suite (unless you fix a bug in the test suite)
    • Change the first number for changes that break the current test suite

Copy link
Member

@mkborregaard mkborregaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really seeing why it's necessary to reimplement here?


DiscreteVoronoi() = DiscreteVoronoi(3)

function _landscape!(mat, alg::DiscreteVoronoi)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this whole function just be

_landscape!(mat, alg::DiscreteVoronoi) = _landscape!(mat, NearestNeighborElement(alg.n, 1))

end of story?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. We probably don't need to reimplement. Just add in documentation that NearestNeighborElement with k set at 1 is voronoi-like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, I was just looking for a way to rasterize a Voronoi (#30). Now I know it's super easy using NearestNeighborElement, we can close the issue 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah no, I think it's great to have a wrapper for this! i just wanted to reuse the function we already had. I think it should work if you edit the PR as suggested :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I wasn't clear what I meant by "reimplement"

end
end

DiscreteVoronoi() = DiscreteVoronoi(3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just add 3 as a default value in the internal constructor in line 9?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nice. I see

"""
DiscreteVoronoi

Assigns a value to each patch using a 1-NN algorithmm with `n` initial clusters
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't explain why it's called "Voronoi"

Copy link
Member

@mkborregaard mkborregaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice @virgile-baudrot looks good - thanks for the contribution!

@mkborregaard mkborregaard merged commit 76db66e into EcoJulia:main Mar 2, 2021
@tpoisot tpoisot mentioned this pull request Mar 3, 2021
@virgile-baudrot virgile-baudrot deleted the voronoi branch March 3, 2021 07:59
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

Successfully merging this pull request may close these issues.

2 participants