Implement Projection classes to encapsulate arguments

[sixy6e]

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.

[welcome]

💖 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]

Suggested change

[leouieda]

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

[sixy6e]

Will remove.

[leouieda]

Suggested change

#!/usr/bin/env python

[leouieda]

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

[sixy6e]

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

[leouieda]

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

[sixy6e]

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]

Suggested change

No empty lines between class/function and docstring.

[sixy6e]

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

[leouieda]

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

[sixy6e]

_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]

_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.

[sixy6e]

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]

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

[sixy6e]

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?

[sixy6e]

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

[sixy6e]

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

[leouieda]

Suggested change

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

[sixy6e]

Good pickup 👍

[sixy6e]

Will fix it.

[MarkWieczorek]

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

[maxrjones]

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]

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]

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]

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]

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.

[sixy6e]

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]

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!