search

conveyal / r5

Developed to power Conveyal's web-based interface for scenario planning and land-use/transport accessibility analysis, R5 is our routing engine for multimodal (transit/bike/walk/car) networks with a particular focus on public transit
https://conveyal.com/learn
MIT License
272 stars 71 forks source link

Merge WGS84 / web Mercator conversion functions #899

Closed abyrd closed 8 months ago

abyrd commented 8 months ago

These functions existed in two places: on Grid and WebMercatorGridPointSet. I retained the more general version that takes a zoom parameter. Math functions were all switched to Java built-in standard Math. We used both FastMath and Java standard Math. FastMath existed because standard Math used to be strict, but Java now has separate StrictMath and standard Math is optimized. This still deserves to be profiled at some point.

I also removed IsochroneFeature which is not used in analysis, but called some of the removed functions. IsochroneFeature was ported to JS as Jsolines, and sees destination travel times as being located at the top left corner of Mercator pixels. Removing it eliminates a source of confusion if someone sees unused code using pixel corners.

This PR is built on top of wgs-mercator-buffer, and is not meant to be merged until other changes are made via the wgs-mercator-* PRs. However, the single commit in this PR can also apply cleanly against the dev branch except for the import statements which determine whether we're using FastMath or Java Math.

abyrd commented 8 months ago

The Github automation is problematic. I like the automatic change of PR base to allow chaining PRs one after another, but it dismisses the reveiws when that happens. People are complaining about this and there's apparently no solution: https://github.com/orgs/community/discussions/57513

abyrd commented 8 months ago

What's the rationale used for importing all the individual static methods (import static ... Math.PI) versus using them directly (Math.PI)?

The short answer is that the code was already statically importing most of the math functions, so when I updated the imports I continued using the same style.

As for why it was originally done that way, I find that the fully qualified names tend to clutter lines that express equations and make them harder to read or verify (look less like the original equations from documentation or papers we're trying to replicate). Unlike detailed variable names, they don't add any clarity to the meaning of the equation, as all the common mathematical functions are widely recognized as coming from Math in a standard library. The use of FastMath as a transparent drop-in was a bit of a Java quirk.

trevorgerhardt commented 8 months ago

The Github automation is problematic. I like the automatic change of PR base to allow chaining PRs one after another, but it dismisses the reveiws when that happens. People are complaining about this and there's apparently no solution: https://github.com/orgs/community/discussions/57513

Would disabling "dismiss stale approvals" fix this?

Screenshot 2023-11-02 at 2 45 07 PM
abyrd commented 8 months ago

The Github automation is problematic. I like the automatic change of PR base to allow chaining PRs one after another, but it dismisses the reveiws when that happens. People are complaining about this and there's apparently no solution: https://github.com/orgs/community/discussions/57513

Would disabling "dismiss stale approvals" fix this?

Yes, but it would also allow adding any number of commits to totally change the PR without further review. Not that we need to rely on these machine-enforced rules, we could just ask for further review when we think it's needed. It's just too bad we need to totally disable the rule / guard rail to allow a common use case.

trevorgerhardt commented 8 months ago

I guess moving from an unprotected branch to a protected one is the key here.

If I create a new unprotected branch off dev, create another branch based off that one, create a PR for merging changes from the second to the first, get approval, then move that PR's base to dev then merging that PR suddenly means something completely different.