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

Tile columns cannot be zero when rounding #2031

Merged
merged 2 commits into from
Feb 28, 2017
Merged

Conversation

snoe925
Copy link
Contributor

@snoe925 snoe925 commented Feb 28, 2017

These round to zero values cause zero column tiles in a LatLng to WebMercator reproject. I could not figure out the root cause. It looked to me like reasonable values were used, and that rounding was the issue. An alternative fix might be to clip the data to the maximum extent of WebMercator in the projection.

Copy link
Member

@lossyrob lossyrob left a comment

Choose a reason for hiding this comment

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

This is a good fix; just looking for a small change.

Have you signed the eclipse CLA with your github email?

val targetCols = math.round(extent.width / cellwidth).toLong
val targetRows = math.round(extent.height / cellheight).toLong
val targetCols = 1L.max(math.round(extent.width / cellwidth).toLong)
val targetRows = 1L.max(math.round(extent.height / cellheight).toLong)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the contribution!

This is a nitpick, but I think using math.max directly would be more optimal.

The 1L.max is a method implicitly added via RichLong (implicit here), which extends ScalaNumberProxy via IntegralProxy[Long], which is an unspecialized class. So, I would assume this would box. However, looking at the byte code, I don't see that it does:

Scala:

  def withImplicit: Long =
    1L.max(2L)

  def withoutImplicit: Long =
    math.max(1L, 2L)

bytecode:

  public long withImplicit();
    Code:
       0: getstatic     #66                 // Field scala/runtime/RichLong$.MODULE$:Lscala/runtime/RichLong$;
       3: getstatic     #71                 // Field scala/Predef$.MODULE$:Lscala/Predef$;
       6: lconst_1
       7: invokevirtual #75                 // Method scala/Predef$.longWrapper:(J)J
      10: ldc2_w        #76                 // long 2l
      13: invokevirtual #81                 // Method scala/runtime/RichLong$.max$extension:(JJ)J
      16: lreturn

  public long withoutImplicit();
    Code:
       0: getstatic     #87                 // Field scala/math/package$.MODULE$:Lscala/math/package$;
       3: lconst_1
       4: ldc2_w        #76                 // long 2l
       7: invokevirtual #90                 // Method scala/math/package$.max:(JJ)J
      10: lreturn

Not sure how this actually doesn't box...(sorry 'thinking out loud' here)

However, we do pay the price of the longWrapper call, which instantiates a class. So, the math.max direct call would be faster than the 1l.max call. By how much? Surely not a lot. But given one of two options, I'll always choose the faster one (and the one that produces less byte code) :)

Can you switch to that method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Eclipse CLA is signed as snoe925@gmail.com

Copy link
Contributor Author

@snoe925 snoe925 left a comment

Choose a reason for hiding this comment

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

Changed to math.max

@lossyrob
Copy link
Member

👍

@lossyrob lossyrob merged commit 3517ac2 into locationtech:master Feb 28, 2017
@lossyrob lossyrob modified the milestones: 1.0.1, 1.1 Mar 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants