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.

Sign up for GitHub

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

Jump to bottom

Add dynamic ZFactor for slope calculation #3014

Merged

pomadchin merged 18 commits into locationtech:master from jbouffard:feature/zfactor-calculator-con

Jul 19, 2019

Contributor

jbouffard commented Jul 8, 2019 •

edited by echeipesh

Loading

Overview

This PR adds the ZFactor class as a way of producing the correct zFactor when calculating slope. Before, the zFactor had a default value of 1. However, this doesn't make sense, as the zFactor needs to be calculated on a per-tile bases. The ZFactor class will help by deriving the appropriate ZFactor for each tile depending on its spatial position.

Checklist

docs/CHANGELOG.rst updated, if necessary
New user API has useful Scaladoc strings
Unit tests added for bug-fix or new feature

Closes #2548

pomadchin added the in progress label

jbouffard added needs review and removed in progress labels

pomadchin requested changes

View reviewed changes

Member

pomadchin left a comment •

edited

Loading

A nice ZFactor port! Some code style questions remaining and there are no headers generated for new files. See https://github.com/sbt/sbt-header#creating-headers

docs/CHANGELOG.rst Outdated Show resolved Hide resolved

layer/src/main/scala/geotrellis/layer/mapalgebra/focal/FocalTileLayerCollectionMethods.scala Outdated Show resolved Hide resolved

spark/src/main/scala/geotrellis/spark/mapalgebra/focal/FocalTileLayerRDDMethods.scala Outdated Show resolved Hide resolved

Contributor Author

jbouffard commented Jul 9, 2019

@pomadchin Thanks! I should've resolved the issues you pointed out. Also, thanks for reminding me about the headers. I knew I forgot something...

echeipesh requested changes

View reviewed changes

Contributor

echeipesh left a comment

Lots of comments about naming. I feel now that this code is being lifted from geopyspark-backend where there was no user interaction its important to reconsider the naming decisions since this is going to be the front and center API for GT scala.

raster/src/main/scala/geotrellis/raster/mapalgebra/focal/Unit.scala Outdated Show resolved Hide resolved

spark/src/main/scala/geotrellis/spark/mapalgebra/focal/FocalOperation.scala Show resolved Hide resolved

spark/src/main/scala/geotrellis/spark/mapalgebra/focal/FocalOperation.scala Show resolved Hide resolved

raster/src/main/scala/geotrellis/raster/mapalgebra/focal/ZFactorCalculator.scala Outdated

                   unit match {
                     case Feet =>
                       ZFactorCalculator((lat: Double) => 1 / (LAT_LNG_FEET_AT_EQUATOR * math.cos(math.toRadians(lat))))
                     case Meters =>
                       ZFactorCalculator((lat: Double) => 1 / (LAT_LNG_METERS_AT_EQUATOR * math.cos(math.toRadians(lat))))
                   }
-                def createZFactorCalculator(mappedLats: Map[Double, Double]): ZFactorCalculator = {
+                def createCalculator(mappedLats: Map[Double, Double]): ZFactorCalculator = {

Contributor

echeipesh Jul 9, 2019

A little pedantic but I still think worth pointing out words like create are generally filler words in function names. Constructors create an object, functions return a value. Unless there is a special significance to the fact that you're getting a new instance, if it has surprisingly high resource requirement for instance, they are unnecessary.

createCalculator is more meaningful as interpolateFromTable - given that interpolation is the critical thing that it does.

createLatLngCalculator is as meaningful and shorter as forLatLng giving you the following full path ZFactorCalculator.forLatLng

raster/src/main/scala/geotrellis/raster/mapalgebra/focal/ZFactorCalculator.scala Outdated Show resolved Hide resolved

raster/src/main/scala/geotrellis/raster/mapalgebra/focal/ZFactorCalculator.scala Outdated Show resolved Hide resolved

Contributor

echeipesh commented Jul 10, 2019

I'm 👍 on squants after realizing there still isn't a standard Java units library.
Just checked, it needs CQ added: https://github.com/locationtech/geotrellis/blob/master/docs/CQ.rst

jbouffard added 17 commits

July 10, 2019 10:44


          Created the ZFactorCaclulator class

b6567da

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Created the Unit types

54dd1b3

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Updated the slope methods for TileLayerCollections and TileLayerRDDs …

207a07b

…to use the ZFactorCalculator

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Updated the tests so that they use the ZFactorCalculator

14d9359

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Added docstrings

dff5eb5

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Renamed Unit to TileUnit

661ac8c

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Updated the CHANGELOG

85200da

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Added ZFactorCalculator, TileUnit, Feet, and Meters to the raster pac…

007af37

…kage object

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Renamed the deriveSlope methods to slope

29784ff

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Impoved the mapOverBufferedTiles method

203924c

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Impoved the docstrings for ZFactorCalculator

991d128

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Fixed typo

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Cleaned up the code

fcd8e0e

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Added headers to new files

3d1e394

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Removed TileUnits and replaced it with the squants library

fe6b8f4

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Renamed ZFactorCalculator to ZFactor as well as rename some of its me…

891453c

…thods

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>


          Cleaned up code

507c783

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

pomadchin added the needs cq label

jbouffard force-pushed the feature/zfactor-calculator-con branch from 2cf3f86 to 507c783 Compare

July 10, 2019 14:45

Contributor Author

jbouffard commented Jul 10, 2019

Just created the CQ for squants: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=20340#c0

echeipesh requested changes

View reviewed changes

layer/src/main/scala/geotrellis/layer/mapalgebra/focal/CollectionFocalOperation.scala Outdated Show resolved Hide resolved

spark/src/main/scala/geotrellis/spark/mapalgebra/focal/FocalOperation.scala Outdated Show resolved Hide resolved

spark/src/main/scala/geotrellis/spark/mapalgebra/focal/FocalOperation.scala Show resolved Hide resolved


          Fixed code

37781f1

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

echeipesh approved these changes

View reviewed changes

echeipesh removed the needs review label

echeipesh changed the title ~~ZFactorCalculator~~ Add dynamic ZFactor for slope calculation

Contributor

echeipesh commented Jul 10, 2019

@jbouffard heads up, you attached the artifact jar instead of the source jar on that CQ request.

pomadchin removed the needs cq label

pomadchin approved these changes

View reviewed changes

pomadchin merged commit c6f424d into locationtech:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet