Skip to content

Add dynamic ZFactor for slope calculation - #3014

Merged
pomadchin merged 18 commits into
locationtech:masterfrom
jbouffard:feature/zfactor-calculator-con
Jul 19, 2019
Merged

Add dynamic ZFactor for slope calculation#3014
pomadchin merged 18 commits into
locationtech:masterfrom
jbouffard:feature/zfactor-calculator-con

Conversation

@jbouffard

@jbouffard jbouffard commented Jul 8, 2019 •
edited by echeipesh
Loading

Copy link
Copy Markdown
Contributor

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
pomadchin requested changes Jul 9, 2019
View reviewed changes

@pomadchin pomadchin left a comment •
edited
Loading

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.

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

Comment thread docs/CHANGELOG.rst Outdated
Comment thread layer/src/main/scala/geotrellis/layer/mapalgebra/focal/FocalTileLayerCollectionMethods.scala Outdated
Comment thread spark/src/main/scala/geotrellis/spark/mapalgebra/focal/FocalTileLayerRDDMethods.scala Outdated
@jbouffard

jbouffard commented Jul 9, 2019

Copy link
Copy Markdown
Contributor Author

@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
echeipesh requested changes Jul 9, 2019
View reviewed changes

@echeipesh echeipesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread raster/src/main/scala/geotrellis/raster/mapalgebra/focal/Unit.scala Outdated
Comment thread spark/src/main/scala/geotrellis/spark/mapalgebra/focal/FocalOperation.scala
Comment thread spark/src/main/scala/geotrellis/spark/mapalgebra/focal/FocalOperation.scala
Comment thread raster/src/main/scala/geotrellis/raster/mapalgebra/focal/ZFactorCalculator.scala Outdated
}

def createZFactorCalculator(mappedLats: Map[Double, Double]): ZFactorCalculator = {
def createCalculator(mappedLats: Map[Double, Double]): ZFactorCalculator = {

@echeipesh echeipesh Jul 9, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment thread raster/src/main/scala/geotrellis/raster/mapalgebra/focal/ZFactorCalculator.scala Outdated
Comment thread raster/src/main/scala/geotrellis/raster/mapalgebra/focal/ZFactorCalculator.scala Outdated
@echeipesh

echeipesh commented Jul 10, 2019

Copy link
Copy Markdown
Contributor

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
@jbouffard
Created the ZFactorCaclulator class
b6567da 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Created the Unit types
54dd1b3 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Updated the slope methods for TileLayerCollections and TileLayerRDDs …
207a07b 
…to use the ZFactorCalculator

Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Updated the tests so that they use the ZFactorCalculator
14d9359 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Added docstrings
dff5eb5 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Renamed Unit to TileUnit
661ac8c 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Updated the CHANGELOG
85200da 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Added ZFactorCalculator, TileUnit, Feet, and Meters to the raster pac…
007af37 
…kage object

Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Renamed the deriveSlope methods to slope
29784ff 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Impoved the mapOverBufferedTiles method
203924c 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Impoved the docstrings for ZFactorCalculator
991d128 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Fixed typo
4457583 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Cleaned up the code
fcd8e0e 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Added headers to new files
3d1e394 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Removed TileUnits and replaced it with the squants library
fe6b8f4 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Renamed ZFactorCalculator to ZFactor as well as rename some of its me…
891453c 
…thods

Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
Cleaned up code
507c783 
Signed-off-by: Jacob Bouffard <[email protected]>
@jbouffard
jbouffard force-pushed the feature/zfactor-calculator-con branch from 2cf3f86 to 507c783 Compare July 10, 2019 14:45
@jbouffard

jbouffard commented Jul 10, 2019

Copy link
Copy Markdown
Contributor Author

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

echeipesh
echeipesh requested changes Jul 10, 2019
View reviewed changes
Comment thread layer/src/main/scala/geotrellis/layer/mapalgebra/focal/CollectionFocalOperation.scala Outdated
Comment thread spark/src/main/scala/geotrellis/spark/mapalgebra/focal/FocalOperation.scala Outdated
Comment thread spark/src/main/scala/geotrellis/spark/mapalgebra/focal/FocalOperation.scala
@jbouffard
Fixed code
37781f1 
Signed-off-by: Jacob Bouffard <[email protected]>
@echeipesh echeipesh changed the title ZFactorCalculator Add dynamic ZFactor for slope calculation Jul 10, 2019
@echeipesh

echeipesh commented Jul 10, 2019

Copy link
Copy Markdown
Contributor

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

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

Reviewers

@echeipesh echeipesh echeipesh approved these changes
@pomadchin pomadchin pomadchin approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Create a Method That Calculates ZFactor

3 participants

@jbouffard @echeipesh @pomadchin