Implement Projection classes to encapsulate arguments #379

sixy6e · 2019-11-18T11:20:15Z

WIP; Create Projection classes instead of strings

This pull request is to address #356 and create projection classes instead of requiring the user to format their own strings which can be cumbersome and complicated.
The desired functionality was to enable tab completion enabling users to immediately see what projections are supported. It is also desirable for the classes to utilise the str method to return a correctly formatted string supported by GMT.

An example of creating an lambert azimuthal equal area projection:

>>> from pygmt import projection
>>> proj = projection.LambertAzimuthalEqualArea(lon0=30, lat0=-20, horizon=60, width="8i")
>>> proj
LambertAzimuthalEqualArea(lon0=30, lat0=-20, horizon=60, width='8i')
>>> print(proj)
A30/-20/60/8i

The supported projections are also defined via an Enum. These act as constants (change in one place only) and serve multiple purposes:

defining and retrieving the projection name
defining and retrieving the GMT code
listing the supported projections
easy boolean comparison of projection labels

All the classes are relying on attrs for simplicity in creation, whilst getting all the benefits that attrs provides such as an easier declaration of validators thus enabling parameter checks for invalid values.

Arguments need to be supplied as keywords (was simpler to define projections classes with mixtures of default values and None for parameters; the benefit is greater clarity to the user and avoiding mixups of incorrect parameter ordering).
The projection classes are also defined to be immutable, acting as a kind of config (this can be changed if desired).

Fixes #356

Reminders

Run make format and make check to make sure the code follows the style guide.
Add tests for new features or tests that would have caught the bug that you're fixing.
Add new public functions/methods/classes to doc/api/index.rst.
Write detailed docstrings for all functions/methods.
If adding new functionality, add an example to docstrings or tutorials.
Finish including the other projections that GMT supports.

…out for a sample of the projections supported by GMT.

welcome · 2019-11-18T11:20:18Z

💖 Thanks for opening this pull request! 💖

Please make sure you read our contributing guidelines and abide by our code of conduct.

A few things to keep in mind:

If you need help writing tests, take a look at the existing ones for inspiration. If you don't know where to start, let us know and we'll walk you through it.
All new features should be documented. It helps to write the docstrings for your functions/classes before writing the code. This will help you think about your code design and results in better code.
No matter what, we are really grateful that you put in the effort to do this! 🎉

leouieda

@sixy6e thank you so much for doing all this work! This is excellent 👍 I have some comments and suggestions below. Let me know if you don't agree with any or have any questions.

I would recommend adding tests and documentation before implementing any more projections. I find it easier to get work on a single one first until we nail the design, tests, and docs. Then you can expand without having to copy changes to all classes. But that's for a next PR 🙂

leouieda · 2019-11-28T10:24:29Z

pygmt/projection.py

@@ -0,0 +1,590 @@
+#!/usr/bin/env python
+


Suggested change

Both lines are unnecessary since this is never run as a script.

Will remove.

leouieda · 2019-11-28T10:24:50Z

pygmt/projection.py

@@ -0,0 +1,590 @@
+#!/usr/bin/env python


Suggested change

#!/usr/bin/env python

leouieda · 2019-11-28T10:25:58Z

pygmt/projection.py

+>>> proj
+LambertAzimuthalEqualArea(central_longitude=30, central_latitude=-20, horizon=60, width=8, unit='i', center=[30, -20])
+>>> print(proj)
+A30/-20/60/8i


Examples are better placed in the tutorials, gallery, or class docstrings. This one will never be rendered in the documentation.

Will remove the module level example. I'll put some examples together in the tutorials section soon.

leouieda · 2019-11-28T10:27:22Z

pygmt/projection.py

+import attr
+
+
+class Supported(Enum):


As mentioned in #356, what is the intended use case for this class? Sorry if I'm being thick 🙂

It's not really needed anymore. I was mostly using it as a place holder for the GMT projection codes, and to keep track (for myself) what had and had not been implemented.
I'll remove it when the projection class module gets closer to completion.

leouieda · 2019-11-28T10:27:57Z

pygmt/projection.py

+
+@attr.s()
+class _Projection:
+


Suggested change

No empty lines between class/function and docstring.

Sorry, my bad. I'll update all places.

pygmt/projection.py

leouieda · 2019-11-28T10:37:37Z

pygmt/projection.py

+
+    def __str__(self):
+        exclude = attr.fields(self.__class__)._fmt
+        kwargs = attr.asdict(self, filter=attr.filters.exclude(exclude))


Does _code or other _ arguments not need to be excluded as well?

_code is required for the string formatter to work. But I thought it best to keep the attribute hidden from the user, and let the class populate by default the required GMT code.

leouieda · 2019-11-28T12:14:20Z

pygmt/projection.py

+
+    # 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 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.

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.

leouieda · 2019-11-28T12:14:56Z

pygmt/projection.py

+        repr=False,
+        default="{_code}{central_longitude}/{central_latitude}/{horizon}/{width}{unit}",
+    )
+    _code: str = attr.ib(init=False, repr=False, default=Supported.UNDEFINED.value)


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

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?

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

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

leouieda · 2019-11-28T12:16:44Z

pygmt/projection.py

+    _fmt: str = attr.ib(
+        init=False,
+        repr=False,
+        default="{_code}{central_longitude}/{central_latitude}/{wdith}{unit}",


Suggested change

default="{_code}{central_longitude}/{central_latitude}/{wdith}{unit}",

default="{_code}{central_longitude}/{central_latitude}/{width}{unit}",

Good pickup 👍

Will fix it.

…centimetres).

…r than use the Enum which will be removed once all projections are defined.

…ert IV, Eckert VI, Van der Grinten, Winkel Tripel, Hammer, Robinson.

…inventer.

…ctions.

MarkWieczorek · 2020-01-13T13:24:03Z

Is this PR about finished? I'd like to start implementing it in pyshtools if it is ready to go...

maxrjones · 2022-06-11T21:33:53Z

Hi @sixy6e, I'm checking in to see whether you're interested in continuing work on this PR, prefer to pass the PR to someone else, or rather abandon the PR for a separate design? For context, I'm giving a talk about PyGMT at SciPy next month and would like to solicit feedback on potential API improvements (including projection classes). Please let me know your preferred path forward 🚀 and thanks for your work on this to date 🙌

maxrjones · 2022-11-22T19:31:46Z

Hi @sixy6e, I'm checking in to see whether you're interested in continuing work on this PR, prefer to pass the PR to someone else, or rather abandon the PR for a separate design? For context, I'm giving a talk about PyGMT at SciPy next month and would like to solicit feedback on potential API improvements (including projection classes). Please let me know your preferred path forward 🚀 and thanks for your work on this to date 🙌

Following up on this @sixy6e because I'm keen to move this forward over this holidays. Unless I hear differently I'll go ahead and build this further as a separate PR.

sixy6e · 2022-11-23T12:03:02Z

Hi @maxrjones

Thanks for the reminder ping. I've been out of action for quite a while with project work.

I'm keen to wrap this work up. I'll do a refresh of where the work was at tomorrow. From memory there were a couple projections left but they might've been a slightly different structure. So will need some input.

sixy6e · 2022-12-18T07:07:19Z

I'm currently adding in more tests. For the output projection string, is there a preference for including the prefix "-J" or leaving it out?

maxrjones · 2022-12-18T15:19:40Z

I'm currently adding in more tests. For the output projection string, is there a preference for including the prefix "-J" or leaving it out?

Nice! My initial reaction would be to leave it out, but open to other suggestions here.

…ed optional params. General cleanup.

…-default params for the different cylindrical projections.

…ns to other projection tests.

sixy6e · 2022-12-21T06:32:11Z

Woops. Sorry @maxrjones

I wasn't thinking, and just auto-piloted with a rebase off "main" as my original branch was quite old without much further thought. Please let me know how you'd like to progress.

I've reset my branch to just before the rebase to avoid including the mass history (i probably should've done git pull --rebase).. But please let me know what you would prefer.

Update as to where things are at. All projections should now be implemented, along with a unittests for each projection.

maxrjones · 2022-12-21T18:49:37Z

I wasn't thinking, and just auto-piloted with a rebase off "main" as my original branch was quite old without much further thought. Please let me know how you'd like to progress.

I've reset my branch to just before the rebase to avoid including the mass history (i probably should've done git pull --rebase).. But please let me know what you would prefer.

We use squash commits when merging any PRs into main, meaning that there are no benefits for the overall project history of using rebase rather than merge for keeping PR branches up to date with main. We also clean up the log that is included in the commit message when merging PRs. So, I recommend merging main into your proj-classes branch either locally and pushing to your fork or using the GitHub 'Update branch' button and then pulling the merge commit to your clone. While git log will show all the commits included in the merge (over 1000 in your case), the GitHub PR interface will only show the last merge commit so it's still easy to track which of the commits are associated with your changes.

Update as to where things are at. All projections should now be implemented, along with a unittests for each projection.

Awesome! I'll take a more detailed look tomorrow - really appreciate your continued work on this!

Josh Sixsmith added 3 commits November 18, 2019 06:30

Initial commit for pygmt/projection.py; Contains a generic design/lay…

0c969bb

…out for a sample of the projections supported by GMT.

Updated docstrings for class projection definitions.

16819cc

Initial run of Black to reformat code.

979d057

vercel bot temporarily deployed to staging November 18, 2019 11:20 Inactive

vercel bot temporarily deployed to staging November 19, 2019 10:50 Inactive

leouieda changed the title ~~Proj classes~~ Implement Projection classes to encapsulate arguments Nov 28, 2019

leouieda mentioned this pull request Nov 28, 2019

Create Projection classes instead of strings #356

Open

leouieda requested changes Nov 28, 2019

View reviewed changes

Josh Sixsmith added 16 commits December 7, 2019 14:38

Renamed lon0 to central_longitude and lat0 to central_latitude.

c1161a3

Added the attribute to give meaning to the width argument (inches or …

29e263d

…centimetres).

Updated example, and cleaned up docstring for unit description.

129ae99

Removed center attribute from printed example.

adef7ad

Specifying the projection code directly in the attrib creation, rathe…

97c836b

…r than use the Enum which will be removed once all projections are defined.

Added the miscellaneous projections group; Mollweide, Sinusoidal, Eck…

a414f7e

…ert IV, Eckert VI, Van der Grinten, Winkel Tripel, Hammer, Robinson.

Capitalised projection names where required, eg when named after the …

c7ee1a2

…inventer.

Added the Polyconic projection.

db7aca6

Added the Miller and oblique 1, 2, 3 projections.

825cd66

Added the Transverse Mercator and Universal Transverse Mercator Proje…

acaca7d

…ctions.

Added the equidistant cylindrical projection.

919102d

Fixed as per @leouieda suggestions.

f5897a8

Missed one of the fixes as suggested by @leouieda

7abe416

Changed the default unit of inches to centimetres.

45abd59

Removed superfluous comments regarding the private variables.

b9997c1

Run Black formatting.

f92c7d1

sixy6e force-pushed the proj-classes branch from a7d885a to f92c7d1 Compare December 8, 2019 09:37

vercel bot temporarily deployed to staging December 8, 2019 09:37 Inactive

Josh Sixsmith added 2 commits January 14, 2020 21:38

Update keyword args for the GeneralPerspective projection.

4161f9e

Initial unittests for the projection class configurations.

968b2cd

vercel bot temporarily deployed to Preview January 14, 2020 10:47 Inactive

MarkWieczorek mentioned this pull request Jan 15, 2020

Improved plotting SHTOOLS/SHTOOLS#209

Merged

MarkWieczorek mentioned this pull request Mar 4, 2020

version 4.6 SHTOOLS/SHTOOLS#220

Merged

weiji14 mentioned this pull request Jun 9, 2020

Need examples for adding texts on maps #469

Closed

weiji14 added the feature Brand new feature label Jun 21, 2020

weiji14 mentioned this pull request Jul 23, 2020

Convenience classes and additional keywords #219

Open

weiji14 force-pushed the master branch from 1730198 to 65b099a Compare February 15, 2021 12:06

seisman mentioned this pull request Mar 20, 2021

What's the most Pythonic way for long GMT arguments? #1082

Open

maxrjones mentioned this pull request Mar 23, 2021

Discuss and document the Pull Request review process #1113

Open

seisman marked this pull request as draft March 24, 2021 23:16

Josh Sixsmith added 2 commits December 18, 2022 17:46

Added Polar and Linear projections. General cleanup.

a77fd13

Apply black formatting

d030593

Josh Sixsmith added 6 commits December 19, 2022 20:51

Various reconfigs; some projs have updated, updated some that specifi…

cd38f61

…ed optional params. General cleanup.

Added a bunch more projections to the test suite.

4793155

Reworked the cylindrical projections to cater for the default and non…

0f24da2

…-default params for the different cylindrical projections.

Added tests for the 3 oblique mercator projection options.

a43d4c8

Added tests for UTM, mercator, equidistant cylindrical. Minor additio…

f39053a

…ns to other projection tests.

Applied black formatting.

f386b09

sixy6e force-pushed the proj-classes branch from 0209474 to f386b09 Compare December 21, 2022 08:12

Josh Sixsmith added 2 commits December 23, 2022 15:34

Merge branch 'main' into proj-classes

8995715

Caught test fails and updated.

20693e7

seisman added this to the 0.12.0 milestone Dec 11, 2023

seisman removed this from the 0.12.0 milestone Feb 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Projection classes to encapsulate arguments #379

Implement Projection classes to encapsulate arguments #379

sixy6e commented Nov 18, 2019

welcome bot commented Nov 18, 2019

leouieda left a comment

leouieda Nov 28, 2019

leouieda Nov 28, 2019

sixy6e Dec 7, 2019

leouieda Nov 28, 2019

leouieda Nov 28, 2019

sixy6e Dec 7, 2019

leouieda Nov 28, 2019

sixy6e Dec 7, 2019

leouieda Nov 28, 2019

sixy6e Dec 7, 2019

leouieda Nov 28, 2019

sixy6e Dec 7, 2019

leouieda Nov 28, 2019

sixy6e Dec 8, 2019

leouieda Nov 28, 2019

sixy6e Dec 7, 2019

sixy6e Dec 7, 2019

sixy6e Dec 7, 2019

leouieda Nov 28, 2019

sixy6e Dec 7, 2019

sixy6e Dec 7, 2019

MarkWieczorek commented Jan 13, 2020

maxrjones commented Jun 11, 2022

maxrjones commented Nov 22, 2022

sixy6e commented Nov 23, 2022

sixy6e commented Dec 18, 2022

maxrjones commented Dec 18, 2022

sixy6e commented Dec 21, 2022 •

edited

Loading

maxrjones commented Dec 21, 2022

	default="{_code}{central_longitude}/{central_latitude}/{wdith}{unit}",
	default="{_code}{central_longitude}/{central_latitude}/{width}{unit}",

Implement Projection classes to encapsulate arguments #379

Are you sure you want to change the base?

Implement Projection classes to encapsulate arguments #379

Conversation

sixy6e commented Nov 18, 2019

welcome bot commented Nov 18, 2019

leouieda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkWieczorek commented Jan 13, 2020

maxrjones commented Jun 11, 2022

maxrjones commented Nov 22, 2022

sixy6e commented Nov 23, 2022

sixy6e commented Dec 18, 2022

maxrjones commented Dec 18, 2022

sixy6e commented Dec 21, 2022 • edited Loading

maxrjones commented Dec 21, 2022

sixy6e commented Dec 21, 2022 •

edited

Loading