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

Add Angle.sinSnap and .cosSnap to avoid small errors, e.g. with buffer operations #1016

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Nov 9, 2023

This is similar to libgeos/geos#978 which aims to remove small rounding errors from Math.sin and Math.cos with (e.g.) pi and pi/2, which are near-zero.

This will provide small differences to buffer operations. For example, consider a 1.0 buffer around POINT (0 0). An intersection with LINESTRING (1 1, 1 -1, -1 -1, -1 1, 1 1) will now be MULTIPOINT ((-1 0), (0 -1), (0 1), (1 0)).

Previously the intersection was MULTIPOINT ((-1 -0.0000000000000001), (-0.0000000000000002 1), (0.0000000000000001 -1), (1 0)).

Several notes:

  • I haven't coded in Java for over 20 years, so better coding hints are welcome
  • Unlike GEOS, Angle.sinCos returns an array. Perhaps there is a better way?
  • Unlike GEOS, there is no BufferOpTest to check the coordinate sequence of POINT(0 0) with a buffer of 1. Is there a suitable place to check this?

@dr-jts
Copy link
Contributor

dr-jts commented Nov 11, 2023

  • Unlike GEOS, Angle.sinCos returns an array. Perhaps there is a better way?

I think two separate functions would be best. Probably more optimal than allocating a new array each time.

  • Unlike GEOS, there is no BufferOpTest to check the coordinate sequence of POINT(0 0) with a buffer of 1. Is there a suitable place to check this?

There is BufferTest. Note that the code there is pretty old. For a more modern pattern for testing the results of operations see this code .

@dr-jts
Copy link
Contributor

dr-jts commented Nov 11, 2023

Is it worth snapping sin and cos values near 0.5 as well?

@mwtoews
Copy link
Contributor Author

mwtoews commented Nov 11, 2023

Do you mean something like public static double clipSin(double ang) and Angle.clipCos? I know that C-based would optimise the pair of functions with FSINCOS, but I'm not sure if Java would/wouldn't do that. It might also not matter (there are many other expensive components to a buffer op than these trig functions).

As for trig values near 0.5, I've not found this to be important. It's only the near-zero ones that matter, which is why I went with the simplest implementation for GEOS. I should also note that this PR would make a buffer around point equal to GEOS (again).

@dr-jts
Copy link
Contributor

dr-jts commented Nov 12, 2023

Do you mean something like public static double clipSin(double ang) and Angle.clipCos? I know that C-based would optimise the pair of functions with FSINCOS, but I'm not sure if Java would/wouldn't do that. It might also not matter (there are many other expensive components to a buffer op than these trig functions).

Yes, that's what I mean. And agreed, I don' think the performance is going to be a bottleneck.

As for trig values near 0.5, I've not found this to be important. It's only the near-zero ones that matter, which is why I went with the simplest implementation for GEOS.

Sounds good.

I should also note that this PR would make a buffer around point equal to GEOS (again).

Excellent, that's what we like to have.

@mwtoews mwtoews changed the title Add Angle::sinCos to avoid small errors, e.g. with buffer operations Add Angle.clipSin and .clipCos to avoid small errors, e.g. with buffer operations Nov 12, 2023
@mwtoews
Copy link
Contributor Author

mwtoews commented Nov 12, 2023

This PR is rewritten as discussed, and ready for further review.

@dr-jts
Copy link
Contributor

dr-jts commented Nov 14, 2023

What do you think about renaming these sinSnap and cosSnap? I think the functionality is more akin to snapping than clipping.

@mwtoews mwtoews changed the title Add Angle.clipSin and .clipCos to avoid small errors, e.g. with buffer operations Add Angle.sinSnap and .cosSnap to avoid small errors, e.g. with buffer operations Nov 15, 2023
@mwtoews
Copy link
Contributor Author

mwtoews commented Nov 15, 2023

Good suggestion. Also, if there are suggestions to push back onto GEOS dev, I can entertain these ideas too.

There are several other parts of the codebase that use Math.sin and .cos that could potentially benefit with clipping near zero, but I haven't gone too far beyond the changes to align it to GEOS.

@dr-jts
Copy link
Contributor

dr-jts commented Nov 15, 2023

Good suggestion. Also, if there are suggestions to push back onto GEOS dev, I can entertain these ideas too.

Might be nice to align GEOS naming with the changed JTS naming.

There are several other parts of the codebase that use Math.sin and .cos that could potentially benefit with clipping near zero, but I haven't gone too far beyond the changes to align it to GEOS.

That can be another PR, sometime.

If there's no more changes required, I can merge this.

@mwtoews
Copy link
Contributor Author

mwtoews commented Nov 15, 2023

Minor GEOS renaming done here: libgeos/geos@dcde8ad

And yes, this PR is now set to merge.

@dr-jts dr-jts merged commit 58d7bd5 into locationtech:master Nov 15, 2023
1 check passed
@mwtoews mwtoews deleted the clipsincos branch November 15, 2023 23:58
@jodygarnett jodygarnett added this to the 1.20.0 milestone Aug 22, 2024
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.

3 participants