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.

Sign up for GitHub

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

Jump to bottom

Implement Projection classes to encapsulate arguments #379

Draft
wants to merge 31 commits into
base: main
Choose a base branch
from
+2,142 −0
Draft

Implement Projection classes to encapsulate arguments #379

Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
0c969bb
Initial commit for pygmt/projection.py; Contains a generic design/lay…
Nov 17, 2019
16819cc
Updated docstrings for class projection definitions.
Nov 18, 2019
979d057
Initial run of Black to reformat code.
Nov 18, 2019
c1161a3
Renamed lon0 to central_longitude and lat0 to central_latitude.
Nov 19, 2019
29e263d
Added the attribute to give meaning to the width argument (inches or…
Nov 19, 2019
129ae99
Updated example, and cleaned up docstring for unit description.
Nov 19, 2019
adef7ad
Removed center attribute from printed example.
Dec 7, 2019
97c836b
Specifying the projection code directly in the attrib creation, rathe…
Dec 7, 2019
a414f7e
Added the miscellaneous projections group; Mollweide, Sinusoidal, Eck…
Dec 7, 2019
c7ee1a2
Capitalised projection names where required, eg when named after the …
Dec 7, 2019
db7aca6
Added the Polyconic projection.
Dec 7, 2019
825cd66
Added the Miller and oblique 1, 2, 3 projections.
Dec 7, 2019
acaca7d
Added the Transverse Mercator and Universal Transverse Mercator Proje…
Dec 7, 2019
919102d
Added the equidistant cylindrical projection.
Dec 7, 2019
f5897a8
Fixed as per @leouieda suggestions.
Dec 7, 2019
7abe416
Missed one of the fixes as suggested by @leouieda
Dec 8, 2019
45abd59
Changed the default unit of inches to centimetres.
Dec 8, 2019
b9997c1
Removed superfluous comments regarding the private variables.
Dec 8, 2019
f92c7d1
Run Black formatting.
Dec 8, 2019
4161f9e
Update keyword args for the GeneralPerspective projection.
Jan 14, 2020
968b2cd
Initial unittests for the projection class configurations.
Jan 14, 2020
a77fd13
Added Polar and Linear projections. General cleanup.
Dec 18, 2022
d030593
Apply black formatting
Dec 18, 2022
cd38f61
Various reconfigs; some projs have updated, updated some that specifi…
Dec 19, 2022
4793155
Added a bunch more projections to the test suite.
Dec 19, 2022
0f24da2
Reworked the cylindrical projections to cater for the default and non…
Dec 20, 2022
a43d4c8
Added tests for the 3 oblique mercator projection options.
Dec 20, 2022
f39053a
Added tests for UTM, mercator, equidistant cylindrical. Minor additio…
Dec 21, 2022
f386b09
Applied black formatting.
Dec 21, 2022
8995715
Merge branch 'main' into proj-classes
Dec 23, 2022
20693e7
Caught test fails and updated.
Dec 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Initial run of Black to reformat code.
  • Loading branch information
Josh Sixsmith committed Nov 18, 2019
commit 979d05710ff1922cd019409cdc4c83ab9db18ef3
87 changes: 49 additions & 38 deletions pygmt/projection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@ class _Projection:

# private; we don't want the user to care or know about
_fmt: str = attr.ib(init=False, repr=False, default="{_code}")
_code: str = attr.ib(init=False, repr=False,
default=Supported.UNDEFINED.value)
_code: str = attr.ib(init=False, repr=False, default=Supported.UNDEFINED.value)
Copy link
Member

Choose a reason for hiding this comment

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

_code and _fmt are the main things that child classes need to specify, right? If so, they should abstract in the base class. To do that, the _Projection and _Azimuthal et al should probably be abstract base classes.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure how well an ABC will work with a child class being defined via attrs. From the docs, attrs trawls the class hierarchy and collects all attributes, and then writes its' own methods to access the required attributes.

So setting an attribute on a child class on a property defined via an ABC fails.

eg:

from abc import ABCMeta, abstractproperty
@attr.s
class _Proj:
    __metaclass__ = ABCMeta

    @abstractproperty
    def longitude(self):
        pass

    @abstractproperty
    def _code(self):
        pass

@attr.s
class MyProj(_Proj):
    longitude: float = attr.ib()
    _code: str = attr.ib(default="X", init=False, repr=False)

Calling:

p = MyProj(145)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<attrs generated init __main__.MyProj-2>", line 3, in __init__
AttributeError: can't set attribute

A way around this would be to create a different attribute and define a property that returns it, but I think it then adds additional complexity to the class definition when you need to define the attribute.

eg

@attr.s
class MyProj(_Proj):
    _longitude = attr.ib()
    __code = attr.ib(default="X", init=False, repr=False)

    @property
    def longitude(self):
        return self._longitude

    @property
    def _code(self):
        return self.__code

Was this along the lines of what you meant when defining the base classes to be an abstract base class?
I'll admit I haven't done much with them since making the move to attrs.


def __str__(self):
maxrjones marked this conversation as resolved.
Show resolved Hide resolved
exclude = attr.fields(self.__class__)._fmt
Expand Down Expand Up @@ -98,10 +97,10 @@ class _Azimuthal(_Projection):
width: str = attr.ib()

# private; we don't want the user to care or know about
_fmt: str = attr.ib(init=False, repr=False,
default="{_code}{lon0}/{lat0}/{horizon}/{width}")
_code: str = attr.ib(init=False, repr=False,
default=Supported.UNDEFINED.value)
_fmt: str = attr.ib(
init=False, repr=False, default="{_code}{lon0}/{lat0}/{horizon}/{width}"
)
_code: str = attr.ib(init=False, repr=False, default=Supported.UNDEFINED.value)
Copy link
Member

Choose a reason for hiding this comment

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

There is probably no need to define _code as "undefined" here. It's what has to be implemented by the base classes, right?

Copy link
Author

Choose a reason for hiding this comment

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

As _Projection, _Azimuthal, _Cylindrical are considered private, I wouldn't expect the user to call them directly. But they could and it would fail if _code is removed, as the _fmt attribute contains {_code}. It is just an empty string placeholder, as such in order to avoid failure from a user calling it explicitly, both the attribute _code and the portion of the str containing {_code} could be removed.
Any thoughts on that approach?

Copy link
Author

Choose a reason for hiding this comment

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

Actually, scratch that, I can't modify the formatter string, as it used by most of the subclasses.

Copy link
Author

Choose a reason for hiding this comment

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

Woops again. I see what you mean. I'll remove it, and maybe I should get to bed ;)


@horizon.validator
def check_horizon(self, attribute, value):
Expand Down Expand Up @@ -133,10 +132,8 @@ class _Cylindrical(_Projection):
width: str = attr.ib()

# private; we don't want the user to care or know about
_fmt: str = attr.ib(init=False, repr=False,
default="{_code}{lon0}/{lat0}/{wdith}")
_code: str = attr.ib(init=False, repr=False,
default=Supported.UNDEFINED.value)
_fmt: str = attr.ib(init=False, repr=False, default="{_code}{lon0}/{lat0}/{wdith}")
_code: str = attr.ib(init=False, repr=False, default=Supported.UNDEFINED.value)


@attr.s(kw_only=True)
Expand Down Expand Up @@ -166,8 +163,9 @@ class _Conic:
width: float = attr.ib()

# private; we don't want the user to care or know about
_fmt: str = attr.ib(init=False, repr=False,
default="{_code}{lon0}/{lat0}/{lat1}/{lat2}/{width}")
_fmt: str = attr.ib(
init=False, repr=False, default="{_code}{lon0}/{lat0}/{lat1}/{lat2}/{width}"
)


@attr.s(frozen=True)
Expand All @@ -189,8 +187,9 @@ class LambertAzimuthalEqualArea(_Azimuthal):
"""

# private; we don't want the user to care or know about
_code: str = attr.ib(init=False, repr=False,
default=Supported.LAMBERT_AZIMUTH_EQUAL_AREA.value)
_code: str = attr.ib(
init=False, repr=False, default=Supported.LAMBERT_AZIMUTH_EQUAL_AREA.value
)


@attr.s(frozen=True)
Expand All @@ -214,8 +213,9 @@ class AzimuthalEquidistant(_Azimuthal):
horizon: float = attr.ib(default=180, kw_only=True)

# private; we don't want the user to care or know about
_code: str = attr.ib(init=False, repr=False,
default=Supported.AZIMUTHAL_EQUIDISTANT.value)
_code: str = attr.ib(
init=False, repr=False, default=Supported.AZIMUTHAL_EQUIDISTANT.value
)


@attr.s(frozen=True)
Expand All @@ -239,8 +239,9 @@ class AzimuthalGnomic(_Azimuthal):
horizon: float = attr.ib(default=60, kw_only=True)

# private; we don't want the user to care or know about
_code: str = attr.ib(init=False, repr=False,
default=Supported.AZIMUTHAL_GNOMIC.value)
_code: str = attr.ib(
init=False, repr=False, default=Supported.AZIMUTHAL_GNOMIC.value
)

@horizon.validator
def check_horizon(self, attribute, value):
Expand Down Expand Up @@ -272,8 +273,9 @@ class AzimuthalOrthographic(_Azimuthal):
horizon: float = attr.ib(default=90)

# private; we don't want the user to care or know about
_code: str = attr.ib(init=False, repr=False,
default=Supported.AZIMUTHAL_ORTHOGRAPHIC.value)
_code: str = attr.ib(
init=False, repr=False, default=Supported.AZIMUTHAL_ORTHOGRAPHIC.value
)

@horizon.validator
def check_horizon(self, attribute, value):
Expand Down Expand Up @@ -323,10 +325,14 @@ class GeneralPerspective(_Projection):
width: float = attr.ib()

# private; we don't want the user to care or know about
_fmt: str = attr.ib(init=False, repr=False,
default="{_code}{lon0}/{lat0}/{altitude}/{azimuth}/{tilt}/{twist}/{viewport_width}/{viewport_height}/{width}")
_code: str = attr.ib(init=False, repr=False,
default=Supported.GENERAL_PERSPECTIVE.value)
_fmt: str = attr.ib(
init=False,
repr=False,
default="{_code}{lon0}/{lat0}/{altitude}/{azimuth}/{tilt}/{twist}/{viewport_width}/{viewport_height}/{width}",
)
_code: str = attr.ib(
init=False, repr=False, default=Supported.GENERAL_PERSPECTIVE.value
)


@attr.s(frozen=True)
Expand All @@ -350,8 +356,9 @@ class GeneralSterographic(_Azimuthal):
horizon: float = attr.ib(default=90, kw_only=True)

# private; we don't want the user to care or know about
_code: str = attr.ib(init=False, repr=False,
default=Supported.GENERAL_STEREOGRAPHIC.value)
_code: str = attr.ib(
init=False, repr=False, default=Supported.GENERAL_STEREOGRAPHIC.value
)

@horizon.validator
def check_horizon(self, attribute, value):
Expand Down Expand Up @@ -383,8 +390,9 @@ class AlbersConicEqualArea(_Conic):
"""

# private; we don't want the user to care or know about
_code: str = attr.ib(init=False, repr=False,
default=Supported.ALBERS_CONIC_EQUAL_AREA.value)
_code: str = attr.ib(
init=False, repr=False, default=Supported.ALBERS_CONIC_EQUAL_AREA.value
)


@attr.s(frozen=True, kw_only=True)
Expand All @@ -408,8 +416,7 @@ class EquidistantConic(_Conic):
"""

# private; we don't want the user to care or know about
_code: str = attr.ib(init=False, repr=False,
default=Supported.EQUIDISTANT_CONIC)
_code: str = attr.ib(init=False, repr=False, default=Supported.EQUIDISTANT_CONIC)


@attr.s(frozen=True)
Expand All @@ -429,8 +436,9 @@ class CassiniCylindrical(_Cylindrical):
"""

# private; we don't want the user to care or know about
_code: str = attr.ib(init=False, repr=False,
default=Supported.CASSINI_CYLINDRICAL.value)
_code: str = attr.ib(
init=False, repr=False, default=Supported.CASSINI_CYLINDRICAL.value
)


@attr.s(frozen=True)
Expand All @@ -453,8 +461,9 @@ class MercatorCylindrical(_Cylindrical):
lat0: float = attr.ib(default=0, kw_only=True)

# private; we don't want the user to care or know about
_code: str = attr.ib(init=False, repr=False,
default=Supported.MERCATOR_CYLINDRICAL.value)
_code: str = attr.ib(
init=False, repr=False, default=Supported.MERCATOR_CYLINDRICAL.value
)


@attr.s(frozen=True)
Expand All @@ -477,8 +486,9 @@ class CylindricalStereographic(_Cylindrical):
lat0: float = attr.ib(default=0, kw_only=True)

# private; we don't want the user to care or know about
_code: str = attr.ib(init=False, repr=False,
default=Supported.CYLINDRICAL_STEROGRAPHIC.value)
_code: str = attr.ib(
init=False, repr=False, default=Supported.CYLINDRICAL_STEROGRAPHIC.value
)


@attr.s(frozen=True)
Expand All @@ -498,5 +508,6 @@ class CylindricalEqualArea(_Cylindrical):
"""

# private; we don't want the user to care or know about
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# private; we don't want the user to care or know about

No need to repeat the same comment everywhere. In fact, _ is commonly used in Python to indicate "private" so there is no need for this comment at all. In general, our rule is that it's better to have code that doesn't need comments.

Copy link
Author

Choose a reason for hiding this comment

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

Fair comment. I'll remove them.

_code: str = attr.ib(init=False, repr=False,
default=Supported.CYLINDRICAL_EQUAL_AREA.value)
_code: str = attr.ib(
init=False, repr=False, default=Supported.CYLINDRICAL_EQUAL_AREA.value
)