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

Rasterize centroids by default #84

Closed
wants to merge 1 commit into from

Conversation

ninsbl
Copy link
Member

@ninsbl ninsbl commented Aug 9, 2019

Not sure if this should be left for GRASS 8 as it might alter results of existing user scripts, but it would be good to have centroids rasterized e.g. also in v.rast.stats by default. And if areas are rasterized by default, it would make sense to make centroids the default too...

@ninsbl ninsbl added the enhancement New feature or request label Aug 9, 2019
@ninsbl ninsbl requested a review from landam August 9, 2019 20:21
@metzm
Copy link
Contributor

metzm commented Aug 21, 2019

Not sure if this is a good idea, it might cause confusion and unexpected results: if several centroids fall into the same cell, the last one that is rasterized wins which is arbitrary. v.to.rast type=centroid,area is working as expected. Rather adjust scripts like v.rast.stats to also rasterize centroids?

@ninsbl
Copy link
Member Author

ninsbl commented Aug 21, 2019

Not sure if this is a good idea, it might cause confusion and unexpected results: if several centroids fall into the same cell, the last one that is rasterized wins which is arbitrary. v.to.rast type=centroid,area is working as expected.

I guess you are right. BTW, does the order (type=centroid,area vs. type=area,centroid) have any effect?

Rather adjust scripts like v.rast.stats to also rasterize centroids?

OK, will have a look at it...

@metzm
Copy link
Contributor

metzm commented Aug 22, 2019

BTW, does the order (type=centroid,area vs. type=area,centroid) have any effect?

No, areas are always rasterized first. After that, points, lines, boundaries, centroids are rasterized in the order in which they are read from the vector map.

@wenzeslaus
Copy link
Member

And if areas are rasterized by default, it would make sense to make centroids the default too...

I don't understand what is the expected output with type="centroid,area". Isn't the centroid inside the area anyway?

@metzm
Copy link
Contributor

metzm commented Nov 28, 2019

I don't understand what is the expected output with type="centroid,area". Isn't the centroid inside the area anyway?

Yes, the centroid is inside the area. If the center point of a cell is not inside the area, that cell is not assigned to that area. However, if the centroid of that area is inside that cell, that cell gets assigned to this centroid. IIUC, this is what this PR is about.

@wenzeslaus
Copy link
Member

I don't understand what is the expected output with type="centroid,area". Isn't the centroid inside the area anyway?

Yes, the centroid is inside the area. If the center point of a cell is not inside the area, that cell is not assigned to that area. However, if the centroid of that area is inside that cell, that cell gets assigned to this centroid. IIUC, this is what this PR is about.

Right. It makes a difference only (?) in case when a cell center is not inside area but centroid happens to be in that cell. In that case that cell is rasterized. Centroids and boundaries in GRASS GIS is what is constructing areas, so unless the centroid has some special meaning in a specific use case, it is just something hidden.

That's why I don't understand why would a user want the behavior as default? Since @ninsbl is suggesting it, I'm guessing there is a case where the behavior is useful, but it does not seem to me it should be the default behavior. Is having centroids in this context an expected default for most users? If centroids, why not boundaries?

@ninsbl
Copy link
Member Author

ninsbl commented Nov 29, 2019

Sorry for not being clear i my answer to Markus. My plan is to look at scripts like v.rast.stats to also rasterize centroids (or at least add the option if it is not present already).
I just kept the PR open for not forgetting that I plan to do that...
If it is preferable to have a ticket, I can open one and close the PR...

@wenzeslaus
Copy link
Member

If it is preferable to have a ticket, I can open one and close the PR...

Please do. It will be more clear. PR is meant to be merged or closed.

@ninsbl
Copy link
Member Author

ninsbl commented Dec 6, 2019

OK, done.
See: https://trac.osgeo.org/grass/ticket/4006

@ninsbl ninsbl closed this Dec 6, 2019
@neteler neteler added this to the 7.8.1 milestone Dec 9, 2021
@neteler neteler added the duplicate This issue or pull request already exists label Dec 9, 2021
@ninsbl ninsbl deleted the v_to_rast_centroid branch October 26, 2022 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants