-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add tests for distance
expression
#9602
Conversation
bfee56e
to
7d4537c
Compare
update ignore list
7d4537c
to
f579e4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zmiao Some of the render tests use large image file sizes or have more complex data sets than looks to be necessary. Can these be simplified or reduced in size? Reduced test cases make debugging easier when something breaks.
"version": 8, | ||
"metadata": { | ||
"test": { | ||
"width": 500, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zmiao This is quite a large size for a render test. Would it be possible to make a smaller test image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention of this test case is to simulate a real world route and to see if it works. That's why the image size is very big, but I will try to decrease the zoom level and minimize the size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context @zmiao. If it's OK to have one larger size image, I do think it would be valuable to have at least one render test that accurately reflects the real world route line use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zmiao I've audited the render tests that are present. I noticed a few issues:
-
I don't think that we have any render tests with
Polygon
as the evaluated feature. Is this sufficiently covered by theMultiPolygon
cases? Same question forPoint
, where we only have one render test with it as the evaluated feature. -
We do not have render test coverage for every combination of geometry as both the input and evaluated feature. For example, there is a test for
LineString
as the evaluated feature withMultiLineString
as input but not forMultiLineString
as the evaluated feature withLineString
as the input, even though the latter is also a valid implementation ofdistance
. -
Is there any way to add multiple geometry types as inputs or evaluated features to a single render test? The above example could be solved by adding a
LineString
input to thelines-to-lines distance
test, for example. If we take this approach, we should come up with a different naming schema for the folders. -
The current naming schema is not consistently applied. Some of them are
"input-to-evaluatedfeature distance"
while others are"evaluatedfeature-to-input distance"
. See below:
evaluated feature | input | test | |
---|---|---|---|
1 | LineString | LineString | line-to-line distance |
2 | LineString | MultiLineString | line-to-lines distance |
3 | MultiLineString | MultiLineString | lines-to-lines distance |
4 | MultiPolygon | MultiLineString | lines-to-polygons distance |
5 | LineString | Point | point-to-line distance |
6 | MultiLineString | Point | point-to-lines distance |
7 | Point | Point | point-to-point distance |
8 | MultiPoint | Point | point-to-points distance |
9 | MultiPoint | LineString | points-to-line distance |
10 | MultiPoint | MultiLineString | points-to-lines distance |
11 | MultiPoint | MultiPoint | points-to-points distance |
12 | MultiPolygon | MultiPoint | points-to-polygons distance |
13 | MultiPolygon | Polygon | polygon-to-polygons distance |
14 | MultiPolygon | MultiPolygon | polygons-to-polygons distance |
"type": "LineString", | ||
"coordinates": [[103.87667655944823, 1.3198734077978092], [103.87895107269286, 1.3198305038395005], [103.88148307800293, 1.320860198634505], [103.88474464416504, 1.3220186047688565], [103.88603210449219, 1.322136590548516], [103.88659000396729, 1.3221794944670043], [103.8875126838684, 1.322190220446512], [103.88829588890076, 1.3222974802390178], [103.88919711112976, 1.3224583699190876], [103.89040946960449, 1.3228123271785267], [103.89161109924317, 1.3230912025594926], [103.89283418655396, 1.3233486259602608], [103.89425039291382, 1.323606049334287], [103.89545202255249, 1.3238205687922547], [103.89678239822388, 1.3240565401745568], [103.89828443527222, 1.3243568673559299], [103.90021562576294, 1.3247644541867095], [103.90227556228638, 1.3251505890171647], [103.9038634300232, 1.3255152718573336], [103.90473783016205, 1.3258102358799509], [103.90523135662079, 1.3259550363872928], [103.90625596046448, 1.326287541223903], [103.90744686126709, 1.3265985940951497], [103.90831589698792, 1.3268989209681999], [103.9093565940857, 1.3272850554657019], [103.9100432395935, 1.327714093725527], [103.91063332557678, 1.3280358723715427], [103.91147017478943, 1.3285185402619937], [103.91221046447754, 1.3288724966550625], [103.91329407691956, 1.3293658903304729], [103.91475319862366, 1.3300738028229565], [103.91640543937683, 1.330856796706907], [103.91727447509766, 1.3312643824659285], [103.91828298568726, 1.3317041459724615]] | ||
} | ||
}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's an extra set of close brackets here that is causing problems with this file
@chloekraw Thank you very much for the thorough review and creating such a clear table! I agree that the tests are insufficient, in order to prove the equivalent evaluation result between the feature and input data, more test cases need to be added. I am working on it! |
Gl-native implementation: mapbox/mapbox-gl-native#16397