PERF: Faster transform direction if already passed an Enum

[greglucas]

Currently, Enum.create() adds quite a bit of overhead to the transform calls, so we probably don't want to call it when unnecessary. Here I added isinstance() checks to handle it in a completely backwards compatible way.

In [1]: from pyproj import Transformer

In [2]: transformer = Transformer.from_crs(2263, 4326, always_xy=True)

In [3]: %timeit transformer.transform(1, 2, direction="forward")
7.96 µs ± 32 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [4]: %timeit transformer.transform(1, 2, direction="FORWARD")
4.18 µs ± 33.9 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [5]: %timeit transformer.transform(1, 2)
3.26 µs ± 21.6 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

I think it might be even nicer to remove Enum.create() entirely and move over to a StrEnum / BaseEnum(str, Enum) and then the dictionary lookup could be either "FORWARD" or TransformDirection.FORWARD with the caveat that you'd lose the case insensitive conversion, but the code would be a bit cleaner everywhere then.

[codecov]

Codecov Report

Merging #1205 (f76dc26) into main (e345729) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1205   +/-   ##
=======================================
  Coverage   96.25%   96.25%           
=======================================
  Files          20       20           
  Lines        1791     1791           
=======================================
  Hits         1724     1724           
  Misses         67       67           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[snowman2]

StrEnum looks nice, but is Python 3.11+.

[snowman2]

I wonder if it would be simpler to add the change there:

pyproj/pyproj/enums.py

Line 14 in e345729

def create(cls, item):

if isinstance(item, cls):
    return item

[greglucas]

Adding the change directly to Enums would be cleaner, but does add a bit of a speed hit still... (I assume going from Cython -> Python -> Cython adds some overhead?)

StrEnum looks nice, but is Python 3.11+.

I think we could get away with just subclassing string here and defining:
class TransformDirection(str, BaseEnum)
there isn't much more to the stdlib implementation:
https://github.com/python/cpython/blob/1cf3d78c92eb07dc09d15cc2e773b0b1b9436825/Lib/enum.py#L1263

I think the question if you want to go this route is whether losing case sensitivity is a deal breaker or not?

[snowman2]

To make the code simpler, thoughts about adding this function:

cdef PJ_DIRECTION get_pj_direction(object direction) except *:
    if not isinstance(direction, TransformDirection):
        direction = TransformDirection.create(direction)
    return _PJ_DIRECTION_MAP[direction]

and doing this:

cdef PJ_DIRECTION pj_direction = get_pj_direction(direction)

[snowman2]

I think we could get away with just subclassing string

Have you tested the performance of this implementation?

[greglucas]

To make the code simpler, thoughts about adding this function:

👍 That seems like it would be nice!

However, I think we should decide on the next comment first.

I think we could get away with just subclassing string

Have you tested the performance of this implementation?

Yes, it is actually faster because you don't need an isinstance check. You can go right into your dictionary with both TransformDirection.FORWARD and the string "FORWARD" because "FORWARD" == TransformDirection.FORWARD and both versions will match the key. (thus negating the need for the lookup above)

Downsides are that "forward" != TransformDirection.Forward so random cased strings won't work (probably good for Enums actually) and the nice error message of valid options are no longer there.

[snowman2]

How much of a performance improvement do you get compared to this PR? I think that if it is significant, I don't think that being case insensitive is as important.

[snowman2]

I am wondering if this would be faster and still allow the error message and support case insensitive input:

cdef PJ_DIRECTION get_pj_direction(object direction) except *:
    try:
         return _PJ_DIRECTION_MAP[direction]
    except KeyError:
        direction = TransformDirection.create(direction)
        return _PJ_DIRECTION_MAP[direction]

[greglucas]

I like your most recent suggestion of the try/except inside a helper function. We can add StrEnum later too if desired.

[snowman2]

Thanks @greglucas 👍