stebogit
commented
7 years ago @barbalex does this happen with v4.7.3?
Closed barbalex closed 6 years ago
barbalex
commented
7 years ago yes, looks exactly the same
stebogit
commented
7 years ago @barbalex please provide some data sample (I mean sample of points that are actually fed to within or inside) that generate the issue, so we can take a look at it 😜
barbalex
commented
7 years ago The code running is:
const result = within(points, filter)where points are:
{
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"properties": {
"TPopId": 2146452464
},
"geometry": {
"type": "Point",
"coordinates": [
8.764992013425216,
47.3637963754393
]
}
},
{
"type": "Feature",
"properties": {
"TPopId": 2146452428
},
"geometry": {
"type": "Point",
"coordinates": [
8.692715305147914,
47.28269957982056
]
}
},
{
"type": "Feature",
"properties": {
"TPopId": 2146452583
},
"geometry": {
"type": "Point",
"coordinates": [
8.182694109987512,
47.811366689874944
]
}
}
]
}and filter is:
{
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "Polygon",
"coordinates": [
[
[
8.670399,
47.253074
],
[
8.670399,
47.255754
],
[
8.67469,
47.255754
],
[
8.67469,
47.253074
],
[
8.670399,
47.253074
]
]
]
}
}
]
}Json created using console.save as explained here: https://stackoverflow.com/a/19818659/712005 And I checked to make sure the error occurs on the mentioned line of code.
rowanwins
commented
7 years ago Hi @barbalex
@turf/inside is designed to only work with single features.
For example this will work
var out = inside(points.features[0], filter.features[0])I'm not sure if there was a change as to how this worked prior to 4.2, @DenisCarriere may know...?
@turf/within should work with feature collections so if you're experiencing errors there you might have to show us how you're using it.
stebogit
commented
7 years ago @barbalex following what @rowanwins said, I don't see any issue with your code and data for @turf/within:
https://jsfiddle.net/jStefano/bzqbm1e9/
barbalex
commented
7 years ago Grrrrr.
My app is running several datasets through within at the same time. Which wasn't clear to me any more :-(
In v4.7.3 the function processing the dataset I gave you worked fine in this corrected version (which I used to extract the data): https://github.com/barbalex/apf2/blob/c478aeccde2b09d940a69d71b74064a30e37cfdb/src/modules/tpopIdsInsideFeatureCollection.js. The error occured at a different dataset.
The reason was that I am using MobX and the points and filters in the functions processing those other datasets were originally not pure objects but MobX observables, i.e. their prototype had been altered. This worked fine in previous versions of Turfjs but not in the recent.
The solution for me was to convert them back to pure objects using toJS from MobX.
I am very sorry to have bugged you with this problem of mine.
I can only hope that this issue may be helpful for someone else using MobX and stumbling on to the same problem.
Thanks a lot for this great tool and the time you put into it.
corinnali
commented
7 years ago Arriving a little late to this discussion (as @barbalex) has closed the issue, but I noticed a similar issue as well (when I realized a previously functional web app broke following the most recent turf update).
For simplicity of demonstration, I extracted the problematic code & JSON objects: in the zip file attached here, are "test_pois.geojson" and "test_buffer.geojson". Both conform to the documented format required for turf.within(). Performing a within (pois, buffer) yields: "Uncaught Error: No valid coordinates"
And, similar to barbalex's situation, my code uses .within() multiple times. It still works for the other instances, but fails for the one case mentioned above.
@stebogit @rowanwins I'm new to issues reporting on Github -- let me know if I should open a new issue for this. Thanks!
rowanwins
commented
7 years ago Hey @corinnali
I notice in your buffer.geojson there are a bunch of features where the geometry is null eg
{"type":"Feature","properties":{},"geometry":null}If I was a gambling man I'd hazard a guess and say this is causing the problem...
We'll look into it further
corinnali
commented
7 years ago Hi @rowanwins -- your guess was spot on! When I added some checks in the code to filter out features with null geom, everything came back to life again.
Thanks to your pointers, I think I know the real cause of this. This buffer object is actually a result of a turf.intersect(), which started returning features with null geom thanks to Fix #820. All's good now! Perhaps would help others if we add a note in the docs cautioning FeatureCollections that contains some features with null geoms for methods such as .within().
stebogit
commented
7 years ago Thanks @corinnali for the info.
@rowanwins @DenisCarriere we should probably make sure @turf/within handles null geometries (ref. https://github.com/Turfjs/turf/pull/866)
rowanwins
commented
7 years ago Hey @stebogit
Im not sure if we should add support for null geometries, that would potentially take us down a path of having to do that for every module. I think my preference would be to add a @turf/cleanFeatureCollection type module that removed any null features, and then put the onus on the user to do their own cleansing... This could perhaps do a bunch of things including removing nulls, but also perhaps cleanCoords etc. I was doing a bit of reading last night about postgis and their IsValid method so stay tuned for an issue with some more ideas thrashed out...
stebogit
commented
7 years ago I see your point, it would probably take time to make sure null geometries are handled correctly by all modules, but in my understanding Turf aims to be as much as possible GeoJSON compliant, so erroring out on null geometries (which are technically valid geometries) is not good. We might only decide to apply this additional "check" from now on to all new modules and intervene on existing modules upon new issues or whenever we need to tweak any of them.
Having said that, modules like @turf/boolean-valid and @turf/clean-feature or @turf/clean-geojson would be definitely useful. 👍
DenisCarriere
commented
7 years ago @barbalex The solution for me was to convert them back to pure objects using toJS from MobX.
MobX observable objects won't play nice with TurfJS modules, unfortunately there's no good solution for this other than the proposed solution to convert back to JS would be your best bet.
@corinnali I'm new to issues reporting on Github -- let me know if I should open a new issue for this. Thanks!
👍Your issue reporting is great! Thanks for submitting your comments 🤗
@rowanwins I think my preference would be to add a @turf/cleanFeatureCollection type module
👎 Don't think there would be much value to develop this kind of module. We shouldn't cause extra iterations over FeatureCollections if it's not needed, it's better to leverage the callback methods in @turf/meta and improve those methods.
@rowanwins Im not sure if we should add support for null geometries, @stebogit I see your point, it would probably take time to make sure null geometries are handled correctly by all modules
A possible solution could be to add an extra optional property (allowNull) in featureEach / featureReduce in @turf/meta to exclude/include null geometries (defaults=false). This would automatically solve a lot of problems from dependent modules as long as those modules uses @turf/meta. We can use the options object (I'll start to refactor a few modules with the new syntax).
This happens in all versions between 4.2.0 and 4.6.0 (it seems that in npm other versions exist than on github).
It happens:
This is the module that runs when the error happens: https://github.com/barbalex/apf2/blob/75ad2a1efc2c57d006e52d93fda8eee1b6d6d9b7/src/modules/tpopIdsInsideFeatureCollection.js
I have checked the data generated for this action several times. There are multiple checks that make sure only valid numbers are passed.
This is the console message: