pygmt icon indicating copy to clipboard operation
pygmt copied to clipboard

Implement Projection classes to encapsulate arguments

Open sixy6e opened this issue 5 years ago • 10 comments

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.

sixy6e avatar Nov 18 '19 11:11 sixy6e

💖 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! 🎉

welcome[bot] avatar Nov 18 '19 11:11 welcome[bot]

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

MarkWieczorek avatar Jan 13 '20 13:01 MarkWieczorek

Sorry @MarkWieczorek not quite there yet. I've been busy of late and haven't had much of chance to do more work on it. Most of the projections are defined, with only Linear and Polar left to do (each has their quirks which complicate things). I've started work on the unittests, and then it'll be the docs and examples.

sixy6e avatar Jan 14 '20 10:01 sixy6e

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 avatar Jun 11 '22 21:06 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.

maxrjones avatar Nov 22 '22 19:11 maxrjones

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 avatar Nov 23 '22 12:11 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?

sixy6e avatar Dec 18 '22 07:12 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?

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

maxrjones avatar Dec 18 '22 15:12 maxrjones

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.

sixy6e avatar Dec 21 '22 06:12 sixy6e

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!

maxrjones avatar Dec 21 '22 18:12 maxrjones