search

OSGeo / PROJ

PROJ - Cartographic Projections and Coordinate Transformations Library
https://proj.org
Other
1.75k stars 789 forks source link

Some suggestions for code cleanup #2387

Closed cffk closed 3 years ago

cffk commented 4 years ago

My recent update to the tmerc documentation #2379 was prompted by queries from NGA (the US National Geospatial Intelligence Agency) figuring what it should do with PROJ. Along the way there was talk of NGA doing a code review of PROJ. Of course, we should welcome this. However, there are many parts of PROJ which are showing their age. Some of these are ripe for updating to improve:

(a) clarity of the code (b) accuracy (c) speed

I've identified the following sections that I'm interested in updating:

  1. documentation for Mercator, merc.rst (currently incomplete)
  2. implementation of Mercator, merc.cpp (inaccurate, slow, odd parity violated)
  3. meridian distance, mlfn.[hc]pp (inaccurate, clumsy inverse)
  4. ellipsoid parameters, ell_set.cpp (inaccurate conversions with trig functions)

I realize that this is a somewhat Sisyphean undertaking. For example, other conformal projections share the defects of the Mercator projection. However, I figure I'd start chipping away at the parts of PROJ that annoy me the most. Even if my changes are reckoned to be too piecemeal, it will be good to capture possible code changes for a future more systematic housecleaning.

I will make separate pull requests for these 4 items and these can be discussed in full at that time. However, holler if you want to know my intentions before I get around to making the changes.

busstoptaktik commented 4 years ago

I figure I'd start chipping away at the parts of PROJ that annoy me the most.

This is awesome, @cffk - scratching your own itch, but leaving the universe in better overall shape as a side effect - a classic driver for open source/free software progress. With a bit of luck and promotion, it will even inspire others to take up the challenge and expand on your work.

kbevers commented 4 years ago

Reopened as #2388 is only a partial fix to this.

billyauhk commented 3 years ago

I found tmerc.cpp was tuned for performance and speed regression on this popular projection will cause concerns at the end users. My suggestion at #2437 might be good for clarity but might be a performance killer (I guess probability is low, but the only way to know is experimentation).

Is there any benchmark and speed which we should try to maintain? Would like to know if that is also included in the CI, and, if there is a to-do list of functions which we may go hunting for performance?

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.