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

Fix geojson-stream integration #10

Merged
merged 1 commit into from
Feb 21, 2019

Conversation

jfrankl
Copy link

@jfrankl jfrankl commented Feb 18, 2019

@andrewharvey I'm brand new to JSON streams, so I apologize if this is an improper approach. This seems to work on my sample geojson multipolygon and polygon data. I would greatly appreciate your feedback.

I was able to recreate the issue you referenced #7 (comment) where exploding multipolygons did not work. The challenge was that when the labelFeature function received a single feature, it would sometimes result in multiple label features.

My approach: within labelFeature, push each label to an array of labeledFeatures. The labeledFeatures array gets returned to the through function. There, we loop through the labeledFeatures and run this.queue for each label.

When determining labels, push each label to an empty array, which gets passed back to the through function. This allows us to use  multiple times when there are multiple labels returned from a single feature.
@andrewharvey
Copy link
Owner

This looks good to me, I'll merge it in but hold off doing a release until we can do further testing

@andrewharvey andrewharvey merged commit a425eae into andrewharvey:geojson-stream Feb 21, 2019
andrewharvey pushed a commit that referenced this pull request Feb 21, 2019
When determining labels, push each label to an empty array, which gets passed back to the through function. This allows us to use  multiple times when there are multiple labels returned from a single feature.
@jfrankl
Copy link
Author

jfrankl commented Feb 21, 2019

@andrewharvey sounds good to me! Can I be of help in testing? For example, if we need 5 GeoJSON that meet certain criteria, I'd be glad to gather or generate what is needed. I'm not sure what is useful, but if there is a task you could delegate to me, I'd be glad to take it on.

@andrewharvey
Copy link
Owner

For the tests I'd prefer as minimal GeoJSON as possible to carry out the test, input and output for:

  • polylabel
  • centroid
  • center-of-mass
  • include-area
  • explode
  • largest
  • combine

If you wanted to put these together or any other tests together, that would be great.

@jfrankl
Copy link
Author

jfrankl commented Feb 21, 2019

@andrewharvey Sure, I'd like to take a shot at this. I don't have much experience writing unit tests, so I could use a little more direction, if you don't mind.

  1. When you say "I'd prefer as minimal GeoJSON as possible", what does that mean? That the test GeoJSON should be small/simple files, or that you are hoping to have a small number of test GeoJSON files?
  2. Any testing libraries or patterns you prefer? Can you point me to any tests you've written on similar projects that I might use as a reference for what you'd like to see?

@andrewharvey
Copy link
Owner

I opened two tickets #11 and #12 that are relevant here.

At the moment you'll see all the code is in bin/. Longer term it's a better approach if the cli code is split from the core labeling code, that would allow for proper unit testing of each method/function.

But until that refactor is in place, I'm okay with simpler testing approach is is just here's a sample input and here's the expected output, and it runs the cli over each of those.

  1. The GeoJSON should be small/simple files, at least to begin with, no limit on the total number of files. That way if there is an issue somewhere it's eaiser to notice and debug with simpler source data. I would just do a single rectangle to start with, then work up to a slightly more complex polygon just enough to show a difference between the labelling algorithms. Could also do two rectangles part of a MultiPolygon, and two rectangles part of a GeometryCollection.

  2. My experience is with https://www.npmjs.com/package/tape and that's what I'd use if we did split cli and core #11 (eg. https://github.com/mapbox/geojson-area). In the mean time I'd just do it a a shell script to run the script over all the inputs and compare with outputs.

@jfrankl jfrankl deleted the geojson-stream branch May 3, 2021 00:06
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