Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #31713 -- Added SpatialReference support to GDALRaster.transform(). #13082

Merged
merged 1 commit into from Jul 7, 2020

Conversation

rico-ci
Copy link
Contributor

@rico-ci rico-ci commented Jun 18, 2020

Deprecated integer SRID as function argument in django.contrib.gis.gdal.GDALRaster.transform in favor of directly passing a SpatialReference item to the transform (django.contrib.gis.gdal.SpatialReference).

Providing patch with updated documentation, release notes, and internal deprecation log.

This is my first PR so please point out anything that can be improved!
Very much appreciated.

@rico-ci rico-ci changed the title Ticket 31713 Fixed #31713 - Changed SRID to SpatialReference in GDALRaster.transform() Jun 18, 2020
@claudep
Copy link
Member

claudep commented Jun 22, 2020

I would be for keeping support for passing an integer SRID. Or do you have a specific reason to prevent it?

@rico-ci
Copy link
Contributor Author

rico-ci commented Jun 22, 2020

My reasoning behind it was that one could pass a SpatialReference object which takes a multitude of inputs anyways. That way the function signature would be clear while keeping a high degree of flexibility. However, there’s even less hassle to keep the int input when thinking of depreciation warnings anyways. So I’m good with either way.

@rico-ci
Copy link
Contributor Author

rico-ci commented Jun 22, 2020

Have a look now. I skipped to support CoordTransform objects. I am not familiar with the cAPI and don't know how I would need to massage the pointer to obtain the target SRID from the target SpatialReference system.

@rico-ci rico-ci changed the title Fixed #31713 - Changed SRID to SpatialReference in GDALRaster.transform() Fixed #31713 - Improved input flexibility for GDALRaster.transform() Jun 22, 2020
Copy link
Member

@claudep claudep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already in good shape in my opinion, thanks!

docs/ref/contrib/gis/gdal.txt Outdated Show resolved Hide resolved
docs/spelling_wordlist Outdated Show resolved Hide resolved
tests/gis_tests/gdal_tests/test_raster.py Outdated Show resolved Hide resolved
@rico-ci
Copy link
Contributor Author

rico-ci commented Jun 25, 2020

Let me know if there's anything else to do here :)

docs/releases/3.2.txt Outdated Show resolved Hide resolved
@claudep
Copy link
Member

claudep commented Jun 25, 2020

Maybe rebase the patch, then wait for a finer review by one of the Django fellows.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rico-ci Thanks for this patch 👍

django/contrib/gis/gdal/raster/source.py Outdated Show resolved Hide resolved
django/contrib/gis/gdal/raster/source.py Outdated Show resolved Hide resolved
django/contrib/gis/gdal/raster/source.py Outdated Show resolved Hide resolved
django/contrib/gis/gdal/raster/source.py Outdated Show resolved Hide resolved
docs/ref/contrib/gis/gdal.txt Outdated Show resolved Hide resolved
docs/ref/contrib/gis/gdal.txt Outdated Show resolved Hide resolved
docs/spelling_wordlist Outdated Show resolved Hide resolved
docs/releases/3.2.txt Outdated Show resolved Hide resolved
docs/releases/3.2.txt Outdated Show resolved Hide resolved
@felixxm felixxm self-assigned this Jul 6, 2020
@felixxm felixxm changed the title Fixed #31713 - Improved input flexibility for GDALRaster.transform() Fixed #31713 -- Added SpatialReference support to GDALRaster.transform(). Jul 7, 2020
@felixxm
Copy link
Member

felixxm commented Jul 7, 2020

@rico-ci Thanks 👍 Welcome aboard ⛵

I used subTest() in tests and removed test related with ticket-31766.

@felixxm felixxm merged commit cb0da63 into django:master Jul 7, 2020
@rico-ci
Copy link
Contributor Author

rico-ci commented Jul 7, 2020

Cheers @felixxm! Was a good first one to get started here.

@rico-ci rico-ci deleted the ticket_31713 branch July 7, 2020 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants