Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor coordEach utilising geomEach internally // alternative approach to #57 #63

Merged
merged 1 commit into from
Mar 16, 2022

Conversation

lukas-h
Copy link
Member

@lukas-h lukas-h commented Mar 8, 2022

As described in my comment here:
#57 (comment)

I have built an implementation where I could get rid of many of the helper functions by implementing it internally with geomEach. I just wanted to test if that is possible, but seems to work. You can find it here:

in response to @tobrun's comment here: #57 (comment)

This PR proposes the refactored code from #50 to match geomEach setup.
[...]
Would you take a different approach?

what do you think?

@lukas-h lukas-h requested a review from tobrun March 9, 2022 07:47
Copy link
Collaborator

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Looks great to me, might be good if we can run the benchmarks for both the old branch as this and see if there is a measurable difference

@lukas-h
Copy link
Member Author

lukas-h commented Mar 11, 2022

I will try to do it!

@lukas-h
Copy link
Member Author

lukas-h commented Mar 14, 2022

Branch lukas-idea-coordeach-refactor:

 coordEach
  ✓ geometry (0 us)
    Elapsed:    2 s
    Rounds:     2274999
    Iterations: 10
  ✓ feature (0 us)
    Elapsed:    2 s
    Rounds:     2581970
    Iterations: 10
  ✓ geometry collection (200 us)
    Elapsed:    2 s
    Rounds:     9994
    Iterations: 10
  ✓ feature collection (218 us)
    Elapsed:    2 s
    Rounds:     9137
    Iterations: 10

Branch tvn-coordeach-refactor:

coordEach
  ✓ geometry (0 us)
    Elapsed:    2 s
    Rounds:     3546893
    Iterations: 10
  ✓ feature (0 us)
    Elapsed:    2 s
    Rounds:     3860529
    Iterations: 10
  ✓ geometry collection (0 us)
    Elapsed:    2 s
    Rounds:     7164767
    Iterations: 10
  ✓ feature collection (102 us)
    Elapsed:    2 s
    Rounds:     19512
    Iterations: 10

Branch main:

 coordEach
  ✓ geometry (0 us)
    Elapsed:    2 s
    Rounds:     3125071
    Iterations: 10
  ✓ feature (0 us)
    Elapsed:    2 s
    Rounds:     3323376
    Iterations: 10
  ✓ geometry collection (0 us)
    Elapsed:    2 s
    Rounds:     3248169
    Iterations: 10
  ✓ feature collection (223 us)
    Elapsed:    2 s
    Rounds:     8942
    Iterations: 10

@tobrun

@lukas-h
Copy link
Member Author

lukas-h commented Mar 14, 2022

At the moment, it looks like, your version @tobrun is the fastest.

But I suspect, that you are skipping GeometryCollection altogether, which causes the 0 us here. I may be wrong (#57 (comment))

@tobrun
Copy link
Collaborator

tobrun commented Mar 15, 2022

Let's go with this version!

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

Successfully merging this pull request may close these issues.

2 participants