Re: [Django] #36058: Refactoring SpatialRefSysMixin.srs for efficiency and better error handling

[Django]

#36058: Refactoring SpatialRefSysMixin.srs for efficiency and better error handling
-------------------------------------+-------------------------------------
Reporter: Arnaldo Govene | Owner: Arnaldo
Type: | Govene
Cleanup/optimization | Status: assigned
Component: GIS | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Arnaldo Govene:

Old description:

> The srs property in the `SpatialRefSysMixin` class has several issues
> that can be addressed to improve code efficiency, maintainability, and
> adherence to best practices. The following points highlight the concerns:
>
> **1. Recursive Calls:**
> The property uses `return self.srs` within the try blocks. This
> introduces recursion, which could lead to stack overflow in edge cases or
> unexpected behavior.
>
> Current Code:
>
> {{{
>
> try:
> self._srs = gdal.SpatialReference(self.wkt)
> return self.srs # Recursive call
> except Exception as e:
> msg = e
> }}}
>
> **2. Error Handling:**
> The msg variable is overwritten in the second try block without being
> utilized after the first block, making error reporting ambiguous. This
> results in the final error message only reflecting the second exception,
> losing context from the first one.
>
> Current Code:
>
> {{{
> try:
> self._srs = gdal.SpatialReference(self.wkt)
> return self.srs
> except Exception as e:
> msg = e # First exception caught
>
> try:
> self._srs = gdal.SpatialReference(self.proj4text)
> return self.srs
> except Exception as e:
> msg = e # Second exception overwrites the first
> }}}
>
> **Proposal:**
>
> **1. Improve Error Reporting:**
> Capture and differentiate between errors raised during the initialization
> from WKT and PROJ.4. Ensure that both error messages are included in the
> final exception for better clarity and debugging.
>
> **2. Raplace Manual Caching with `@cached_propert` and Remove Recursive
> Calls:**
> To further optimize the `srs` property, replace the manual caching
> mechanism with `@cached_property`. This simplifies the code while
> retaining the benefits of caching, which is particularly important given
> the computational cost of creating a gdal.SpatialReference object.
>
> **Refactored Code:**
>
> {{{
> from django.utils.functional import cached_property
>
> class SpatialRefSysMixin:
>
> @cached_property
> def srs(self):
> """
> Return a GDAL SpatialReference object.
> """
> try:
> return gdal.SpatialReference(self.wkt)
> except Exception as e:
> wkt_error = f"Error initializing from WKT: {str(e)}"
>
> try:
> return gdal.SpatialReference(self.proj4text)
> except Exception as e:
> proj4_error = f"Error initializing from PROJ.4: {str(e)}"
>
> raise Exception(f"Could not get OSR SpatialReference. WKT Error:
> {wkt_error} | PROJ.4 Error: {proj4_error}")
> }}}
>
> **Rationale for Using cached_property:**
> - Caches the computationally expensive creation of
> `gdal.SpatialReference`, improving efficiency for repeated access.
> - Removes manual caching logic, making the code cleaner and easier to
> maintain.
> - Ideal for mostly static spatial reference data, avoiding unnecessary
> recomputation.

New description:

The srs property in the `SpatialRefSysMixin` class has several issues that
can be addressed to improve code efficiency, maintainability, and
adherence to best practices. The following points highlight the concerns:

**1. Recursive Calls:**
The property uses `return self.srs` within the try blocks. This introduces
recursion, which could lead to stack overflow in edge cases or unexpected
behavior.

Current Code:

{{{

try:
self._srs = gdal.SpatialReference(self.wkt)
return self.srs # Recursive call
except Exception as e:
msg = e
}}}

**2. Error Handling:**
The msg variable is overwritten in the second try block without being
utilized after the first block, making error reporting ambiguous. This
results in the final error message only reflecting the second exception,
losing context from the first one.

Current Code:

{{{
try:
self._srs = gdal.SpatialReference(self.wkt)
return self.srs
except Exception as e:
msg = e # First exception caught

try:
self._srs = gdal.SpatialReference(self.proj4text)
return self.srs
except Exception as e:
msg = e # Second exception overwrites the first
}}}

**Proposal:**

**1. Improve Error Reporting:**
Capture and differentiate between errors raised during the initialization
from WKT and PROJ.4. Ensure that both error messages are included in the
final exception for better clarity and debugging.

**2. Raplace Manual Caching with `@cached_propert` and Remove Recursive
Calls:**
To further optimize the `srs` property, replace the manual caching
mechanism with `@cached_property`. This simplifies the code while
retaining the benefits of caching, which is particularly important given
the computational cost of creating a gdal.SpatialReference object.

**Refactored Code:**

{{{
from django.utils.functional import cached_property

class SpatialRefSysMixin:

@cached_property
def srs(self):
"""
Return a GDAL SpatialReference object.
"""
try:
return gdal.SpatialReference(self.wkt)
except Exception as e:
wkt_error = f"Error initializing from WKT: {str(e)}"

try:
return gdal.SpatialReference(self.proj4text)
except Exception as e:
proj4_error = f"Error initializing from PROJ.4: {str(e)}"

raise Exception(f"Could not get OSR SpatialReference. WKT Error:
{wkt_error} | PROJ.4 Error: {proj4_error}")
}}}

**Rationale for Using cached_property:**
- Caches the computationally expensive creation of
`gdal.SpatialReference`, improving efficiency for repeated access.
- Removes manual caching logic, making the code cleaner and easier to
maintain.
- Ideal for mostly static spatial reference data, avoiding unnecessary
recomputation.

Forum discussion: https://forum.djangoproject.com/t/caching-strategy-for-
spatialrefsysmixin-srs/37634/3

--
--
Ticket URL: <https://code.djangoproject.com/ticket/36058#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36058: Refactoring SpatialRefSysMixin.srs for efficiency and better error handling
-------------------------------------+-------------------------------------
Reporter: Arnaldo Govene | Owner: Arnaldo
Type: | Govene
Cleanup/optimization | Status: assigned
Component: GIS | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Arnaldo Govene):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36058#comment:5>

[Django]

#36058: Refactoring SpatialRefSysMixin.srs for efficiency and better error handling
-------------------------------------+-------------------------------------
Reporter: Arnaldo Govene | Owner: Arnaldo
Type: | Govene
Cleanup/optimization | Status: assigned
Component: GIS | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36058#comment:6>

[Django]

#36058: Refactoring SpatialRefSysMixin.srs for efficiency and better error handling
-------------------------------------+-------------------------------------
Reporter: Arnaldo Govene | Owner: Arnaldo
Type: | Govene
Cleanup/optimization | Status: assigned
Component: GIS | Version: 5.1
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36058#comment:7>

[Django]

#36058: Refactoring SpatialRefSysMixin.srs for efficiency and better error handling
-------------------------------------+-------------------------------------
Reporter: Arnaldo Govene | Owner: Arnaldo
Type: | Govene

Cleanup/optimization | Status: closed
Component: GIS | Version: 5.1
Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"8ff1399f0656b4ff8181fe7872e426c331d1b1ee" 8ff1399]:
{{{#!CommitTicketReference repository=""
revision="8ff1399f0656b4ff8181fe7872e426c331d1b1ee"
Fixed #36058 -- Refactored SpatialRefSysMixin.srs to use cached_property.

Replaced manual caching complexity with cached_property for efficiency.
Enhanced error handling with distinct messages for WKT and PROJ.4.

Thanks to Sarah Boyce for the suggestions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36058#comment:8>