Skip to content

updated computeResolution in IterativeCostDistance to match Iterative Viewshed - #3106

Merged
pomadchin merged 3 commits into
locationtech:masterfrom
jmtaysom:master
Oct 3, 2019
Merged

updated computeResolution in IterativeCostDistance to match Iterative Viewshed#3106
pomadchin merged 3 commits into
locationtech:masterfrom
jmtaysom:master

Conversation

@jmtaysom

@jmtaysom jmtaysom commented Oct 2, 2019 •
edited by pomadchin
Loading

Copy link
Copy Markdown
Contributor

The compute resolution method in Iterative Cost Distance was calcultating the
cell size based on the length of a degree at the equator without scaling for
the current latitude. This introduces an error as you move away from the
equator. The same method in Iterative Viewshed compensates for this and
maintains greater spatial accuracy.

See issue #3103

Signed-off-by: jmtaysom [email protected]

Overview

The compute resolution method in Iterative Cost Distance was calcultating the
cell size based on the length of a degree at the equator without scaling for
the current latitude. This introduces an error as you move away from the
equator. The same method in Iterative Viewshed compensates for this and
maintains greater spatial accuracy.

Checklist

  • docs/CHANGELOG.rst updated, if necessary

Closes #3103

@jmtaysom

jmtaysom commented Oct 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

There are two tests that failed https://github.com/locationtech/geotrellis/blob/master/spark/src/test/scala/geotrellis/spark/costdistance/IterativeCostDistanceSpec.scala#L100 and https://github.com/locationtech/geotrellis/blob/master/spark/src/test/scala/geotrellis/spark/costdistance/IterativeCostDistanceSpec.scala#L110

The two previous tests that are very similar passed

[info] - Should propogate left (1 second, 176 milliseconds)
[info] - Should propogate right (1 second, 227 milliseconds)
[info] - Should propogate up *** FAILED *** (1 second, 198 milliseconds)
[info]   5.000000000000001 was not equal to 5.0 (IterativeCostDistanceSpec.scala:107)
[info] - Should propogate down *** FAILED *** (1 second, 218 milliseconds)
[info]   5.000000000000001 was not equal to 5.0 (IterativeCostDistanceSpec.scala:117)

It looks like a floating point rounding error at first glance but I am not sure how precise the results should be. @echeipesh any suggestions on fixing this?

@pomadchin

pomadchin commented Oct 2, 2019

Copy link
Copy Markdown
Member

hey @jmtaysom you can fix these tests by writing smth like hops should be (5.0 +- 1e-10).
Also, mb it makes sense to put computeResolution function into some object CostDistance that we can have in a spark package?

@pomadchin

pomadchin commented Oct 2, 2019

Copy link
Copy Markdown
Member

Anyway, even without this code shuffling, I will 👍 the PR once travis is happy. GZ with your first contribution!

@jmtaysom

jmtaysom commented Oct 3, 2019

Copy link
Copy Markdown
Contributor Author

@pomadchin All the tests are passing now

@pomadchin
pomadchin self-requested a review October 3, 2019 16:43
@pomadchin pomadchin self-assigned this Oct 3, 2019
@jmtaysom @pomadchin
updated computeResolution in IterativeCostDistance to match Iterative…
2b3e4b0 
…Viewshed

The compute resolution method in Iterative Cost Distance was calcultating the
cell size based on the length of a degree at the equator without scaling for
the current latitude. This introduces an error as you move away from the
equator. The same method in Iterative Viewshed compensates for this and
maintains greater spatial accuracy.

See issue 3103 locationtech#3103

Signed-off-by: jmtaysom <[email protected]>
@jmtaysom @pomadchin
updated cost distance tests for compute resolution
12599ec 
Signed-off-by: jmtaysom <[email protected]>
@pomadchin
pomadchin force-pushed the master branch 2 times, most recently from 23ab5bc to 0fed8a9 Compare October 3, 2019 20:08
pomadchin
pomadchin reviewed Oct 3, 2019
View reviewed changes
Comment thread spark/src/main/scala/geotrellis/spark/costdistance/IterativeCostDistance.scala
/**
* Compute the resolution (in meters per pixel) of a layer.
*/
private [spark] def computeResolution[K: (* => SpatialKey), V: (* => Tile)](

@pomadchin pomadchin Oct 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I decided to move this function into the CostDistance object to avoid code duplication (we had two versions of the same function implemented in two different places).

pomadchin
pomadchin approved these changes Oct 3, 2019
View reviewed changes

@pomadchin pomadchin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Merging once travis is happy.

@pomadchin
pomadchin merged commit 82878cb into locationtech:master Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pomadchin pomadchin pomadchin approved these changes

Assignees

@pomadchin pomadchin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update computeResolution in cost distance

2 participants

@jmtaysom @pomadchin