XY refactor #11
Merged
XY refactor #11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Plan is stop using tuples that might contain X,Y or Y,X ordered values and instead be explicit about the order during construction and use.
Codecov Report
@@ Coverage Diff @@
## develop #11 +/- ##
===========================================
+ Coverage 97.13% 97.44% +0.31%
===========================================
Files 13 14 +1
Lines 1639 1841 +202
===========================================
+ Hits 1592 1794 +202
Misses 47 47
Continue to review full report at Codecov.
|
`Geobox(width, height, ...)` interface is problematic for several reasons: 1. not compatible with `ndarray`, where shape is used instead 2. no type-safety, easy to get order wrong 3. two parameters to configure single property Replacing `width, height` with a single parameter `shape`. This could be a simple `(int, int)` tuple for easier interop with `ndarray`. But one can choose to use more typesafe `XY[int]`. Example of the change at call site ```python # before change: GeoBox(512, 256, A, "espg:4326") # becomes ## if array order is "native" at call-site GeoBox((256, 512), A, "epsg:4326") ## if width/height orded is preferred GeoBox(ixy_(512, 256), A, "epsg:4326") ```
Allow conversion from tuples and passthrough of output types.
Instead of using plain tuples use XY classes.
Kirill888
force-pushed
the
xy-refactor
branch
from
f8c94d9
to
27183ef
Compare
Use XY[] objects internally and stop indexing with `[0|1]`. Do not accept plain tuples for Resolution/Alignment Report resolution/alignment as Resolution/XY objects
Kirill888
commented
Feb 4, 2022
gadomski
requested changes
Feb 7, 2022
Kirill888
commented
Feb 7, 2022
After change to XY[] comments about axis order are no longer relevant.
nx,ny instead of W,H
s{x,y}, t{x,y} for Affine matrix components
adding some clarification comments.
- using x twice instead of x,y - adding test with non-square tiles
those were missed in error
It behavse like a tuple of ny,nx int values, but also is XY[int]
geobox/gridspec use Shape2d now
Kirill888
force-pushed
the
xy-refactor
branch
from
e4ad35b
to
1a17224
Compare
add work around for crs<->geom dependency. enable warnings are errors mode for doc building.
Kirill888
force-pushed
the
xy-refactor
branch
from
9e2234a
to
c342091
Compare
|
since your last review:
|
gadomski
approved these changes
Feb 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were a bunch of places where it was not clear whether some tuple contains X,Y or Y,X data. It's confusing on the interface and confusing when reading code. This PR adds an
XY[T]type and a bunch of constructor methods that make it easy to understand what order x,y parameters are added.Instead of using plain tuples those new types are used instead. On the input
shape=parameters are still allowed to be supplied as plain(int, int)tuples and are equivalent toiyx_(y, x). Internally those properties are kept inXY[int]form. Similarly a 2d index is also allowed to be supplied as(int, int), as this is what allowssome_thing[ix, iy]orsome_thing[row, col]syntax, order depends on use case, but should be clearly stated in the docs.Resolution input parameters are allowed to be a single float, but plain tuples are not allowed as this causes order confusion. Single value resolution is equivalent to
resyx_(-value, value).Change GeoBox constructor to accept shape
Geobox(width, height, ...)interface is problematic for severalreasons:
ndarray, where shape is used insteadReplacing
width, heightwith a single parametershape. This could be a simple(int, int)tuple for easier interop withndarray. But one can choose to use more typesafeXY[int].Example of the change at call site