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

40% faster turf-area #2520

Merged
merged 7 commits into from
Nov 14, 2023
Merged

40% faster turf-area #2520

merged 7 commits into from
Nov 14, 2023

Conversation

AminoffZ
Copy link
Contributor

This is a continuation on #1387, again only modifying the function ringArea(coords: number[][]): number. Fixed the issue with turf-isobands. Had to REGEN=true for turf-nearest-neighbor-analysis. There were some floating point differences on random-large-study-area.json where area(studyArea) would return

1396158428.635545

instead of

1396158428.6355453

Bench

Before

polygon x 5,999,571 ops/sec ±0.37% (94 runs sampled)

After

polygon x 8,510,024 ops/sec ±0.28% (96 runs sampled)

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Have read How To Contribute.
  • Run npm test at the sub modules where changes have occurred.
  • Run npm run lint to ensure code style at the turf module level.

@twelch
Copy link
Collaborator

twelch commented Oct 22, 2023

Thanks @AminoffZ, full test suite in CI is running now.

After a quick initial review, when I read your comment "Fixed the issue with turf-isobands. Had to REGEN=true for turf-nearest-neighbor-analysis", I was expecting to see code in your PR somewhere related to it, but it wasn't clear to me. Can you say more about this comment and any code changes for it? Perhaps you possibly forgot to commit some of the code? Thanks.

@AminoffZ
Copy link
Contributor Author

AminoffZ commented Oct 22, 2023

EDIT: The turf-isobands was failing its test in the PR this is derived from. #1387
Hi, the fixes for turf-isobands is in 7c5d382 along with some additional speed improvements. TBH I'm not entirely familiar with the testing process, but plainly, what I did was set REGEN=true when running the turf-nearest-neighbor-analysis test, which made it pass when it did not before. The result of running that was is in the last two commits.

@twelch
Copy link
Collaborator

twelch commented Oct 22, 2023

Thank you. I guess I was expecting the REGEN=true to be a code change as well. I can see now there is a level to the repo/testing I don't understand yet either.

Copy link
Collaborator

@twelch twelch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @AminoffZ for this contribution, I approve. I see now that you used REGEN=true to update the test outputs, which look very very close to the previous.

@twelch
Copy link
Collaborator

twelch commented Nov 14, 2023

@AminoffZ before I merge, can you go ahead and update the performance benchmark in turf-area/bench.js. I don't currently see a way to do this other than manually editing. I know the speed will be relative to your machine, that's okay for now.

@twelch twelch merged commit c8b37e8 into Turfjs:master Nov 14, 2023
3 checks passed
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.

None yet

2 participants