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

r.centroids #396

Merged
merged 14 commits into from Mar 12, 2021
Merged

r.centroids #396

merged 14 commits into from Mar 12, 2021

Conversation

chaedri
Copy link
Contributor

@chaedri chaedri commented Jan 8, 2021

Added r.centroids, a wrapper module for r.volume that computes centroids from clumps of data as a raster.

@wenzeslaus wenzeslaus added the new addon PR contains a new addon or issue proposes one label Jan 8, 2021
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

This looks great and it has a potential to be quite useful. I like how short it is.

Well, it is so short that I didn't have much to say, but I did some nit-picking.

You can also run flake8, pylint, and black on it. It will flag things such as #% which you can't do anything about right now, but it will further polish the code.

grass7/raster/r.centroids/r.centroids.html Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.py Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.py Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.py Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.py Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.py Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.py Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.html Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.html Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.html Outdated Show resolved Hide resolved
Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

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

I was also recommending looking at the attribute table of the output vector - it might be good idea to drop some of the columns which don't make sense for r.centroids.

grass7/raster/r.centroids/r.centroids.html Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.py Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.py Outdated Show resolved Hide resolved
chaedri and others added 7 commits January 25, 2021 16:59
Co-authored-by: Markus Neteler <neteler@osgeo.org>
Co-authored-by: Markus Neteler <neteler@osgeo.org>
Co-authored-by: Markus Neteler <neteler@osgeo.org>
Co-authored-by: Markus Neteler <neteler@osgeo.org>
Changed to match naming convention.
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Some new, some old, and some newly noticed things.

grass7/raster/r.centroids/r.centroids.html Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.py Outdated Show resolved Hide resolved
grass7/raster/r.centroids/r.centroids.py Outdated Show resolved Hide resolved
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Thank you for making all the adjustments!

@wenzeslaus wenzeslaus merged commit 66d901e into OSGeo:master Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new addon PR contains a new addon or issue proposes one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants