Opened 4 years ago

Closed 3 years ago

#2658 closed defect (fixed)

[PATCH] v.to.db compactness gives wrong results in feet location

Reported by: annakrat Owned by: grass-dev@…
Priority: normal Milestone: 7.0.5
Component: LibGIS Version: svn-releasebranch70
Keywords: v.to.db, units, area, compactness Cc:
CPU: Unspecified Platform: All

Description

I was looking in the source code of v.to.db to understand if computed area is in meters squared or location units (feet in my case). I realized area is converted to meters squared, however, perimeter seems to be in map units (feet). Therefore compactness is wrong when units are not meters (latlon is fine I guess). Setting feet as units doesn't influence compactness computation.

Currently the library functions for computing area and perimeter don't behave consistently.

Attachments (1)

v_to_db_length_units.diff (1.6 KB) - added by mlennert 4 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 in reply to:  description Changed 4 years ago by mlennert

Summary: v.to.db compactness gives wrong results in feet location[PATCH] v.to.db compactness gives wrong results in feet location

Replying to annakrat:

I was looking in the source code of v.to.db to understand if computed area is in meters squared or location units (feet in my case). I realized area is converted to meters squared, however, perimeter seems to be in map units (feet). Therefore compactness is wrong when units are not meters (latlon is fine I guess). Setting feet as units doesn't influence compactness computation.

Currently the library functions for computing area and perimeter don't behave consistently.

And perimeter information is wrong when using unit=feet parameter:

g.region s=0 n=10 w=0 e=10 res=1
v.mkgrid map=mygrid box=1,1

Then, when asking for perimeter without unit parameter, you get the correct perimeter in feet:

> v.to.db -p mygrid op=perimeter
Reading areas...
 100%
cat|perimeter
1|4
2|4
3|4
[...]

However, when you explicitly ask for feet, the result is wrong since v.to.db assumes that the perimeter is in meters and converts to feet:

> v.to.db -p mygrid op=perimeter unit=feet
Reading areas...
 100%
cat|perimeter
1|13.1233595800525
2|13.1233595800525
3|13.1233595800525
[...]

Currently, v.to.db seems to assume that all length and area values are in meters by default and converts them to the requested unit. For now I suggest to respect that assumption, unless the user explicitly asks for another unit. I'll attach a patch which attempts to do this. Please test.

The only inconvenience is that when the user asks for the original units (e.g. feet) the values are approximate only. Example for a 5 feet line:

> v.to.db -p line op=length unit=feet
Reading features...
 100%
cat|length
1|5.00001000001999

The other (probably cleaner) option would be to create a new function for area calculation that returns area in location units and modify v.to.db to always return location units. However this demands a bit more modification of v.to.db.

In any case, whatever we chose, this will probably break scripts that rely on the existing behavior of v.to.db (even though I consider that behavior as a clear bug).

Changed 4 years ago by mlennert

Attachment: v_to_db_length_units.diff added

comment:2 Changed 4 years ago by neteler

Milestone: 7.0.17.0.2

Ticket retargeted after 7.0.1 milestone closed

comment:3 Changed 4 years ago by neteler

Milestone: 7.0.27.0.3

Ticket retargeted after milestone closed

comment:4 Changed 4 years ago by neteler

Will you apply the patch?

comment:5 in reply to:  4 ; Changed 4 years ago by annakrat

Replying to neteler:

Will you apply the patch?

I applied the patch, but needs more testing.

comment:6 in reply to:  5 ; Changed 4 years ago by mlennert

Replying to annakrat:

Replying to neteler:

Will you apply the patch?

I applied the patch, but needs more testing.

As mentioned, the patch has some side effects in terms of weird results in terms of decimals.

I agree that it is better than leaving as is, but in the long run, changing area calculations to always return current location units might a better idea.

This would entail changing G_area_of_polygon() and this will probably be quite invasive as any library function or module calling this function would have to be updated.

In the meantime this should be backported IMHO.

Moritz

comment:7 in reply to:  6 Changed 4 years ago by annakrat

Priority: majornormal

Replying to mlennert:

Replying to annakrat:

Replying to neteler:

Will you apply the patch?

I applied the patch, but needs more testing.

As mentioned, the patch has some side effects in terms of weird results in terms of decimals.

I agree that it is better than leaving as is, but in the long run, changing area calculations to always return current location units might a better idea.

This would entail changing G_area_of_polygon() and this will probably be quite invasive as any library function or module calling this function would have to be updated.

In the meantime this should be backported IMHO.

Done in r67327.

comment:8 Changed 4 years ago by neteler

Milestone: 7.0.3

Ticket retargeted after milestone closed

comment:9 Changed 4 years ago by neteler

Milestone: 7.0.4

Ticket retargeted after 7.0.3 milestone closed

comment:10 Changed 3 years ago by martinl

Milestone: 7.0.47.0.5

comment:11 Changed 3 years ago by martinl

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.