Eliminating redefined constant, restructuring "earth" package and distributing methods

[benguild]

I noticed this. Seeing as there's a central definition for S2Earth here (as just "earth/") it's a one-liner. see #239 (comment)

[jmr]

Why not replace all uses of earthRadiusKm with earth.Radius.Kilometers()?

It seems like kmToAngle should also have an earth-based replacement.

@flwyd

[benguild]

Why not replace all uses of earthRadiusKm with earth.Radius.Kilometers()?

It seems like kmToAngle should also have an earth-based replacement.

@flwyd

I went ahead and tried to clean that up, but realized "earth" imported "s2" and "s1" which from my first impression does not make sense because then it's unusable from "s2" while still requiring it underneath. I dug into the discussion and ironically this had already been raised as an issue: #191 (comment)

If folks disagree, please feel free to close the PR and keep the duplicate implementation/constant, but I've proposed keeping the constants in "earth" where they're independent and moving the methods/tests that require the types to their respective packages with prefixing.

[jmr]

This is going to break all users in its current form, so we don't want to do that.

[benguild]

This is going to break all users in its current form, so we don't want to do that.

I think you should maybe reconsider, or at least discuss further to make sure our options here are exhausted. That code has only been merged since July and the evidence of the problem here is in the codebase itself: the S2 tests contain reimplemented versions of earth functionality (e.g., kmToAngle(…)).

How much of the earth code will need to live on in a duplicate form in the S2 tests, with its own test(s)? When tests need to duplicate a package's functionality to avoid importing it (hitting an import cycle), that's a clear signal the dependency direction is inverted. Philosophically, Earth's geometry has existed far longer than S1/S2 and so it doesn't make sense that the other packages can't use it for anything, only external importers?

In terms of breaking other uses, this code is months old… not years. The cost of fixing now seems minimal compared to accumulating technical debt which is evident in the tests here and could reappear in future tests. The functionality is still there for importers, but the method calls would need to be adjusted from earth.… to s2.Earth… etc. which seems reasonable if the code's only been present since July, and working with the output of the methods would still require importing the packages that'd now contain them.

[jmr]

@rsned @flwyd have more context on this.

Breaking the API will cause us quite some pain inside Google, but it could be worth it if it's a bad API.

[rsned]

I think it's cleaner to have earth be a separate package. s1/s2 is about spherical geometry on a unit sphere. Earth is an up-scaled approximation of that.

We are stuck with Go's rules on imports. We could reduce some parts of the duplication.

For kmToAngle, we only seem to use specifically 10km in tests. Maybe a simpler clean up would be to have one test constant that is precomputed for 10km with a note on where / how its computed and replace all the kmToAngle calls with that

contains_point_query_test.go: maxLoopRadius := kmToAngle(10)
edge_query_test.go:var testCapRadius = kmToAngle(10)
edge_query_test.go: radius := kmToAngle(bmOpts.radiusKm.Radians())
edge_query_test.go: return s1.Angle(fraction) * kmToAngle(radiusKm)
loop_test.go: loops[i] = RegularLoop(randomPoint(), kmToAngle(10.0), vertices)
pointcompression_test.go: pts := regularPoints(center, kmToAngle(radiusKM), nvertices)
regioncoverer_test.go: regions[i] = RegularLoop(randomPoint(), kmToAngle(10.0), size)

The other tests that rely on the constant could be similarly re-written to not need it.

s2/predicates_test.go: m := math.Tan(spacing / earthRadiusKm)
This one could use the same constant as the above tests.

s2/point_test.go: if dist := math.Acos(OriginPoint().Z) * earthRadiusKm; dist <= 50 {
This could probably be re-written to not do the multiply and just change the dist <= 50 to be dist <= 0.01

s2/polygon_test.go: RegularLoop(origin, 1000/earthRadiusKm, 100),
This tests a loop inversion doesn't contain the original origin, but its just making a loop around it of a random size, it could just as easily pass in 0.5 and the test works the same.

[benguild]

I think where we're running into problems is that if S2 does not have earth in scope then…

• how does S2Earth fit into the new scope when it's present in C++ and elsewhere (parity question) and provides those constants/methods
• why are the S2 tests on Earth values at all if it's not about Earth, and not some general sphere/planet instead?

Also, I again just want to emphasize that it seems a bit weird for earth to depend on S1/S2 when people who want earth may not want S1/S2. The methods that are relative to S1/S2 seem like they belong in S1/S2 and then should import earth as a general concept.

If we're going to lean hard into the earth as a "wrapper" for S1/S2 and any other package (which has bad code smell to me) … I can see an alternative path forward that:

• generalizes the tests in S2 into not being about Earth at all (but that distances us away from the parity with C++ etc.)
• moves any Earth specific tests to earth which can use S1/S2

… But, I'm not sure if that makes sense because then earth is banished from ever being used by any other package it depends on in the repo, and tests about S2 are then in earth if they are actually about both Earth and S2.

@rsned Are you comfortable with S2's tests being generalized away from Earth to enforce the separation, and do you think this is a good direction for the library to take given how S2 is structured in other languages?

For kmToAngle, we only seem to use specifically 10km in tests.

In the diff there are some meters/kilometers seemingly…
https://github.com/golang/geo/pull/239/files#diff-b7d540961857aebecaa719da0eb0d8cedc96fe8c11a95aecb87bbe630e8708d4L870

This was cleaned up to use the clearer units throughout:
https://github.com/golang/geo/pull/239/files#diff-b7d540961857aebecaa719da0eb0d8cedc96fe8c11a95aecb87bbe630e8708d4R886
https://github.com/golang/geo/pull/239/files#diff-fbe2c9af9f192a4e0490fee820bfb1fbbf6da4b5cdc9915c802f4b16cafade71R258

[jmr]

• how does S2Earth fit into the new scope when it's present in C++ and elsewhere (parity question) and provides those constants/methods

I don't understand the question. The functions are available. What are you asking?

• why are the S2 tests on Earth values at all if it's not about Earth, and not some general sphere/planet instead?

It's easier for the people writing the tests, since they're familiar with the Earth.

[benguild]

• how does S2Earth fit into the new scope when it's present in C++ and elsewhere (parity question) and provides those constants/methods

I don't understand the question. The functions are available. What are you asking?

Right, the question is, if "earth" is being split out from S2 here, is that the goal for the libraries across other languages in general? (to separate S2 from being Earth-bound and therefore be purely sphere-focused, splitting off the S2Earth type there into some other package/library, too?)

• why are the S2 tests on Earth values at all if it's not about Earth, and not some general sphere/planet instead?

It's easier for the people writing the tests, since they're familiar with the Earth.

In this case, I would say that further justifies the PR in its current format given that earth should be usable when testing S2, or any other package/library. If earth is going to be continually importing other libraries (S1/S2) as a convenience, that increasingly prevents that otherwise, and seems like an antipattern.

earth is serving two purposes currently… a convenience wrapper and a "functional definition" of the planet and its geometry. As a minimum, the functional definition (radius, etc. as a constant) should be available in all contexts because it has no dependency on S1/S2.

However, the current structure (that is being debated) blocks those constants from being used due to the dependency, and highlights that other elements of earth should be usable by what they depend on. Therefore, they should be relocated to the packages that avoid the import cycle, which is similar to how S2 C++ is structured, unless that structure is changing. (first question)

[jmr]

Right, the question is, if "earth" is being split out from S2 here, is that the goal for the libraries across other languages in general?

It's not a comparable situation, since the other languages don't have "s1" and "s2" packages.

In this case, I would say that further justifies the PR in its current format given that earth should be usable when testing S2, or any other package/library.

The amount of code in s1/s2 tests compared to the rest of the world is small. Doesn't seem like a big deal.

[benguild]

The amount of code in s1/s2 tests compared to the rest of the world is small.

Aside from the fact the code was already deployed… is that the justification for earth import being blocked from S1/S2 for any purpose, and repeating code/calculations from earth in the S2 tests? It's based on a Go limitation, but the expectation is that the import cascade is correct and Earth itself applies to far more than S1/S2.

Again, it seems weird that earth depends on other packages.