Implement Projection classes to encapsulate arguments#379
Implement Projection classes to encapsulate arguments#379sixy6e wants to merge 31 commits intoGenericMappingTools:mainfrom
Conversation
…out for a sample of the projections supported by GMT.
|
💖 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:
|
leouieda
left a comment
There was a problem hiding this comment.
@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 🙂
pygmt/projection.py
Outdated
| @@ -0,0 +1,590 @@ | |||
| #!/usr/bin/env python | |||
|
|
|||
There was a problem hiding this comment.
Both lines are unnecessary since this is never run as a script.
pygmt/projection.py
Outdated
| @@ -0,0 +1,590 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
| #!/usr/bin/env python |
pygmt/projection.py
Outdated
| >>> proj | ||
| LambertAzimuthalEqualArea(central_longitude=30, central_latitude=-20, horizon=60, width=8, unit='i', center=[30, -20]) | ||
| >>> print(proj) | ||
| A30/-20/60/8i |
There was a problem hiding this comment.
Examples are better placed in the tutorials, gallery, or class docstrings. This one will never be rendered in the documentation.
There was a problem hiding this comment.
Will remove the module level example. I'll put some examples together in the tutorials section soon.
pygmt/projection.py
Outdated
| import attr | ||
|
|
||
|
|
||
| class Supported(Enum): |
There was a problem hiding this comment.
As mentioned in #356, what is the intended use case for this class? Sorry if I'm being thick 🙂
There was a problem hiding this comment.
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.
pygmt/projection.py
Outdated
|
|
||
| @attr.s() | ||
| class _Projection: | ||
|
|
There was a problem hiding this comment.
No empty lines between class/function and docstring.
There was a problem hiding this comment.
Sorry, my bad. I'll update all places.
|
|
||
| def __str__(self): | ||
| exclude = attr.fields(self.__class__)._fmt | ||
| kwargs = attr.asdict(self, filter=attr.filters.exclude(exclude)) |
There was a problem hiding this comment.
Does _code or other _ arguments not need to be excluded as well?
There was a problem hiding this comment.
_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.
pygmt/projection.py
Outdated
|
|
||
| # 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) |
There was a problem hiding this comment.
_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.
There was a problem hiding this comment.
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 attributeA 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.__codeWas 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.
pygmt/projection.py
Outdated
| repr=False, | ||
| default="{_code}{central_longitude}/{central_latitude}/{horizon}/{width}{unit}", | ||
| ) | ||
| _code: str = attr.ib(init=False, repr=False, default=Supported.UNDEFINED.value) |
There was a problem hiding this comment.
There is probably no need to define _code as "undefined" here. It's what has to be implemented by the base classes, right?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Actually, scratch that, I can't modify the formatter string, as it used by most of the subclasses.
There was a problem hiding this comment.
Woops again. I see what you mean. I'll remove it, and maybe I should get to bed ;)
pygmt/projection.py
Outdated
| _fmt: str = attr.ib( | ||
| init=False, | ||
| repr=False, | ||
| default="{_code}{central_longitude}/{central_latitude}/{wdith}{unit}", |
There was a problem hiding this comment.
| default="{_code}{central_longitude}/{central_latitude}/{wdith}{unit}", | |
| default="{_code}{central_longitude}/{central_latitude}/{width}{unit}", |
…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.
|
Is this PR about finished? I'd like to start implementing it in pyshtools if it is ready to go... |
|
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. |
|
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. |
|
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.
|
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. |
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.
Awesome! I'll take a more detailed look tomorrow - really appreciate your continued work on this! |
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:
The supported projections are also defined via an Enum. These act as constants (change in one place only) and serve multiple purposes:
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
make formatandmake checkto make sure the code follows the style guide.doc/api/index.rst.