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

updating @turf/inside or @turf/within from v4.1 to v4.2 causes "coordinates must only contain numbers" error #940

Closed
barbalex opened this issue Sep 12, 2017 · 14 comments
Labels

Comments

@barbalex
Copy link

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:

  • if I only update within
  • if I only update inside
  • when I update both

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:

index.js:81 Uncaught Error: coordinates must only contain numbers
    at containsNumber (index.js:81)
    at getCoords (index.js:58)
    at ./node_modules/@turf/within/node_modules/@turf/inside/index.js.module.exports (index.js:37)
    at ./node_modules/@turf/within/index.js.module.exports (index.js:44)
    at ./src/modules/tpopIdsInsideFeatureCollection.js.__webpack_exports__.a (tpopIdsInsideFeatureCollection.js:50)
    at Object../src/store/extend/map.js.__webpack_exports__.a.Object.mapFilter.tpop.Object.name (map.js:168)
    at trackDerivedFunction (mobx.module.js:2812)
    at ComputedValue../node_modules/mobx/lib/mobx.module.js.ComputedValue.computeValue (mobx.module.js:1407)
    at ComputedValue../node_modules/mobx/lib/mobx.module.js.ComputedValue.trackAndCompute (mobx.module.js:1397)
    at ComputedValue../node_modules/mobx/lib/mobx.module.js.ComputedValue.get (mobx.module.js:1360)
    at shouldCompute (mobx.module.js:2763)
    at ComputedValue../node_modules/mobx/lib/mobx.module.js.ComputedValue.get (mobx.module.js:1359)
    at shouldCompute (mobx.module.js:2763)
    at ComputedValue../node_modules/mobx/lib/mobx.module.js.ComputedValue.get (mobx.module.js:1359)
    at Object.get (mobx.module.js:1584)
    at myChildren (index.js:135)
    at _class.Projekte (index.js:176)
    at _class.render (index.js:753)
    at Object.allowStateChanges (mobx.module.js:921)
    at index.js:645
    at trackDerivedFunction (mobx.module.js:2812)
    at Reaction../node_modules/mobx/lib/mobx.module.js.Reaction.track (mobx.module.js:2980)
    at _class.reactiveRender [as render] (index.js:640)
    at ReactCompositeComponent.js:795
    at measureLifeCyclePerf (ReactCompositeComponent.js:75)
    at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (ReactCompositeComponent.js:794)
    at ReactCompositeComponentWrapper._renderValidatedComponent (ReactCompositeComponent.js:821)
    at ReactCompositeComponentWrapper._updateRenderedComponent (ReactCompositeComponent.js:745)
    at ReactCompositeComponentWrapper._performComponentUpdate (ReactCompositeComponent.js:723)
    at ReactCompositeComponentWrapper.updateComponent (ReactCompositeComponent.js:644)
    at ReactCompositeComponentWrapper.performUpdateIfNecessary (ReactCompositeComponent.js:560)
    at Object.performUpdateIfNecessary (ReactReconciler.js:156)
    at runBatchedUpdates (ReactUpdates.js:150)
    at ReactReconcileTransaction.perform (Transaction.js:143)
    at ReactUpdatesFlushTransaction.perform (Transaction.js:143)
    at ReactUpdatesFlushTransaction.perform (ReactUpdates.js:89)
    at Object.flushBatchedUpdates (ReactUpdates.js:172)
    at ReactDefaultBatchingStrategyTransaction.closeAll (Transaction.js:209)
    at ReactDefaultBatchingStrategyTransaction.perform (Transaction.js:156)
    at Object.batchedUpdates (ReactDefaultBatchingStrategy.js:62)
    at batchedUpdates (ReactUpdates.js:97)
    at reactionScheduler (mobx.module.js:3090)
    at runReactions (mobx.module.js:3066)
    at endBatch (mobx.module.js:2621)
    at endAction (mobx.module.js:899)
    at executeAction (mobx.module.js:866)
    at Object.res (mobx.module.js:854)
    at NewClass.<anonymous> (DrawControl.js:83)
    at NewClass.fire (leaflet-src.js:588)
    at NewClass._fireCreatedEvent (leaflet.draw.js:8)
@stebogit
Copy link
Collaborator

@barbalex does this happen with v4.7.3?

@barbalex
Copy link
Author

yes, looks exactly the same

@stebogit
Copy link
Collaborator

@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
Copy link
Author

barbalex commented Sep 12, 2017

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
Copy link
Member

rowanwins commented Sep 13, 2017

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
Copy link
Collaborator

@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
Copy link
Author

barbalex commented Sep 13, 2017

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
Copy link

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!

test.zip

@rowanwins
Copy link
Member

rowanwins commented Sep 13, 2017

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

@rowanwins rowanwins reopened this Sep 13, 2017
@corinnali
Copy link

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
Copy link
Collaborator

Thanks @corinnali for the info.
@rowanwins @DenisCarriere we should probably make sure @turf/within handles null geometries (ref. #866)

@stebogit stebogit added the bug label Sep 14, 2017
@rowanwins
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants