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.

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

Create Projection classes instead of strings #356

Open
leouieda opened this issue Oct 31, 2019 · 15 comments · May be fixed by #379
Open

Create Projection classes instead of strings #356

leouieda opened this issue Oct 31, 2019 · 15 comments · May be fixed by #379
Labels
feature request New feature wanted help wanted Helping hands are appreciated

Comments

@leouieda
Copy link
Member

Description of the desired feature

The Figure methods take a projection argument that is using the same arguments as GMT -J ("M10i" for Mercator 10 inches). But a lot of projections have many arguments and it's impossible to remember them all. A relatively simple alternative would be to implement classes (based on a single Projection class maybe) that take the projection arguments in the constructor (and so allows tab-completion). The key would be for these classes to define a __str__ method so that when str is called on them, they produce the GMT -J code. For example:

>>> proj = pygmt.Orthographic(central_longitude=10, central_latitude=52, width=10)
>>> print(str(proj))
G10/52/10

The plotting method pre-processing would only have to call str on projection, which works fine if you pass in a string as well.

Are you willing to help implement and maintain this feature? Yes but I'd welcome anyone else who wants to try :)

@weiji14
Copy link
Member

weiji14 commented Nov 8, 2019

@leouieda, just mentioning that @MarkWieczorek has done some side work here to parse a human readable projection name into a GMT compatible projection string. It would be good to combine efforts and have that in-house in PyGMT.

Also had a thought about the implementation aspect of this, and it feels like dataclasses would be a nice way to go. It's meant for Python 3.7+ but there's a backport to Python 3.6 at https://github.com/ericvsmith/dataclasses we could pull in. Alternatively, we could use good ol' classes, namedtuples, or maybe the attrs library.

Not sure if it's better to have pygmt.Orthographic() or pygmt.Projection(projection="orthographic"). The first one might clutter up our namespace. Or maybe we could do pygmt.Projection.orthographic? I'd take a look at cartopy.

@leouieda
Copy link
Member Author

@weiji14 I would prefer to avoid having to use a backport as that complicated the packaging quite a bit by introducing different dependencies on different Python versions. This is such a light weight implementation that attrs is probably the best tool for the job.

The problem with pygmt.Projection(projection="orthographic") is that projections take slightly different parameters so this implementation would have to use **kwargs which can't be tab-completed (negating the effect of having the class in the first place). With pygmt.Orthographic(), the projection name and arguments can be tab-completed.

To avoid cluttering the namespace, I'd be fine with bundling them in a submodule like pygmt.projection.Orthographic(). That makes it easier to find as well since you can type pygmt.projection.<TAB> and get a list of all projections.

@leouieda
Copy link
Member Author

I definitely like @MarkWieczorek's names for the projections. Though since we have classes they should be CamelCase.

@weiji14 weiji14 added the help wanted Helping hands are appreciated label Nov 14, 2019
@weiji14
Copy link
Member

weiji14 commented Nov 14, 2019

Ok noted - basically do pygmt.projection.Xxxxx(). I'll see if I can get some people to work on this in about 12 hours at the FOSS4G Oceania Community Day.

@sixy6e
Copy link

sixy6e commented Nov 14, 2019

Hi everyone, I'm just having a quick exploration into this issue whilst at the FOSS4G Oceania Community Day.

I think attrs is a good idea, so I'll prototype out something over the weekend and see what you think. I have used attrs in a similar fashion to what is being described in this issue.

@weiji14 weiji14 moved this from To Do to In Progress in FOSS4G 2019 Community Day Nov 14, 2019
@sixy6e
Copy link

sixy6e commented Nov 17, 2019

I've put together a working prototype for a dozen of the 31 supported GMT projections. You can find the code here (I can do an initial PR as a WorkInProgess if that's easier for an initial review and discussion).

What it enables:

  • pygmt.projection.Xxxxx()
  • listing of supported projections
list(pygmt.projection.Supported)
  • Automatic conversion to GMT's string format via str
  • Keyword arguments that can be validated eg latitude/longitude bounds, horizon limits etc, (currently I've only added validators for the horizon argument).

For any parameters listed as optional on GMT, such as horizon, I've set defaults based on the examples in the pygmt gallery.

The Enum class Supported has multiple purposes such as easy listing of supported projections it also enables a simpler comparison of projection labels. eg

>>> a = Supported.LAMBERT_AZIMUTH_EQUAL_AREA
>>> a == a
True
>>> b = Supported.EQUIDISTANT_CONIC
>>> a == b
False

If such comparisons are desired elsewhere in the code, an additional attribute can be added to the Projection class, thus allowing the Projection class to be carried around to other parts of the pygmt ecosystem.
Each enum instance also contains the projection name and the GMT code as given in GMT.

>>> a = Supported.LAMBERT_AZIMUTH_EQUAL_AREA
>>> a.name
LAMBERT_AZIMUTH_EQUAL_AREA
>>> a.value
A

Anyway, take a look and see what you think. From the descriptions in this thread, it is pretty close to the desired outcomes. If it is along the lines of what you're after I can open up a PR and finish the rest of it off.

Just to note, the names for the keyword arguments have come directly from GMT. These can easily be changed to central_longitude, central_latitude, etc.

@weiji14
Copy link
Member

weiji14 commented Nov 17, 2019

Good work @sixy6e, you must have taken a lot of time over the weekend to get this up! I've taken a quick look already, and I do like the default arguments, but we'll need to discuss that with the rest of the team. Also, the central_longitude does looks better than lon0, and I just realized that the Julia wrapper has done some work already on Map projections and we might want to coordinate a bit on those keyword argument names.

Definitely recommend opening up a draft Pull Request so that we can comment on individual lines of the code and follow up on the discussion there. Before you do that though, have a quick look over our Contributing Guidelines. In particular, you might want to run make format to Black format the code (it's designed to make code diffs more minimal).

@MarkWieczorek
Copy link
Contributor

Just a couple minor comments:

  1. I would probably use central_longitude and central_latitude, but then also define center which has the default value of [central_longitude, central_latitude]
  2. For the width parameter, I would also define an optional unit parameter, which could be either i (default) or c. The gmt width string would then be str(width) + unit.
  3. My preference would be to used mixed case for the projection names, instead of upper case. For ease of use, it might be useful to define purely lowercase and uppercase names as aliases.
  4. Lastly, I think it would be useful to define short name aliases for the projections. The julia project already has given an initial try at this, and the majority of their aliases are good.

@sixy6e sixy6e linked a pull request Nov 18, 2019 that will close this issue
6 tasks
@sixy6e
Copy link

sixy6e commented Nov 18, 2019

Thanks for the feedback @weiji14 and @MarkWieczorek
I'll make the changes to use central_longitude and central_latitude instead of lon0 and lat0.
With center, I'll write it so that the attribute gets created post init, and is then not required to be input by the user.
I was thinking last night that the Enum Supported, is probably better to be called GMTCodes, as it acts as a reference to the shorthand (mostly single character) codes that GMT uses, but I'm open to suggestions. Enum's themselves are typically UPPERCASE to make it clear that they're constants, but by no means, are you forced to follow it either.

@MarkWieczorek
Copy link
Contributor

I only mentioned center because it appears that the Julia-gmt project uses this instead of separate variables for the latitude and longitude. I think that it might be useful to accept either center or both central_longitude and central_latitude.

@leouieda
Copy link
Member Author

leouieda commented Nov 28, 2019

@sixy6e thank you for doing all of this!

I was thinking last night that the Enum Supported, is probably better to be called GMTCodes, as it acts as a reference to the shorthand (mostly single character) codes that GMT uses, but I'm open to suggestions.

What is the rationale for the Supported class? I feel like that is a tab-complete or documentation lookup away. I kind of want to stay away from the single character codes altogether. Ideally, we should support all GMT projections so it's only a matter of time before Supported is obsolete. In that case, is it worth adding all the code (and tests/documentation) required?

I'll leave some more comments in #379.

@MarkWieczorek a few comments to your points:

I would probably use central_longitude and central_latitude, but then also define center which has the default value of [central_longitude, central_latitude]

I really don't like having two arguments for setting the same thing. That makes documentation and testing much more complicated than it needs to be and is confusing for new users. I see the appeal of center but the problem is that not all projections take both central_latitude and central_longitude. So there is no way of center being consistent, which is why I prefer central_latitude and central_longitude.

For the width parameter, I would also define an optional unit parameter, which could be either i (default) or c. The gmt width string would then be str(width) + unit.

That is a great idea! I have having to specify a string for something that should be a number ("10c"). Having a unit="c" in the projections is great!

My preference would be to used mixed case for the projection names, instead of upper case. For ease of use, it might be useful to define purely lowercase and uppercase names as aliases.

Classes should always be CamelCase but module-level constants are UPPER_CASE.

Lastly, I think it would be useful to define short name aliases for the projections. The julia project already has given an initial try at this, and the majority of their aliases are good.

I'm firmly against this. The main point of having these classes is to be explicit with the projection names. Having short lowercase class names breaks the naming conventions. For short-hand, users can always pass projection="W180/10c" instead of using the classes. Plus, there is always tab-completion to avoid typing long names.

@sixy6e
Copy link

sixy6e commented Nov 29, 2019

Hi @leouieda

Thanks for your comments.

The rationale for the Supported class was initially to serve as a configuration kickstarter for a user when defining a projection. As the class system evolved, it simply became a placeholder to hold the GMT code name along with a clear long-form of the projection name. I don't really see it as being that useful anymore (except for a quick listing of projections supported by GMT), as the GMT code name can simply be defined within the class, and abstracted from the user. As it currently stands in dev form, I'm mostly using it to keep track of what projections have been implemented (I hadn't implemented all as I thought it best to wait for additional feedback before I went too far). As such I'm keen to remove it.

I agree with your comment on the center argument. Separate arguments are at least clearly defined. I was starting to think about tests that would involve checking for cases when a user has supplied both forms; which one would be correct. The code would be more complex, and a pain to maintain. As it currently stands, I've only incorporated it as a derived attribute and not as a user input. But it doesn't serve any purpose and ends up cluttering class, and requires explicit removal when generating the proj string (explicit separate args make it easier to define the proj string as well). I can remove center in the next commit.

Very good point. I was taking look through the julia project, and some of the short names were not as immediately recognisable to me. Being explicit provides clarity, and leaves less room for ambiguity. I'm fine to implement whichever the pygmt team desire, but as a user, I'm in favour of clarity.

I'll have some spare time this weekend, so I'll remove the Supported class, and the center attribute, and add a few more projections. I'll try add some tests very soon, as well as updated examples of use.

@willschlitzer
Copy link
Contributor

@sixy6e @MarkWieczorek @weiji14 @leouieda It looks like this issue has been sitting for a little over a year. Has there been any progress on it? I think it's a great idea to get away from having a (somewhat confusing) GMT string to set the projection.

@MarkWieczorek
Copy link
Contributor

I definitely would like to use this feature when it is implemented! Unfortunately, I really doubt that I will have time to contribute any coding over the next couple of months.

@sixy6e
Copy link

sixy6e commented Feb 12, 2021

I'll try to squish some time in and push through the last bits of the this over the next few weeks. Spare time completely disappeared for me the past year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature wanted help wanted Helping hands are appreciated
Projects
Development

Successfully merging a pull request may close this issue.

5 participants