Open leouieda opened 5 years ago
@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.
@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.
I definitely like @MarkWieczorek's names for the projections. Though since we have classes they should be CamelCase.
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.
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.
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:
list(pygmt.projection.Supported)
str
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.
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).
Just a couple minor comments:
central_longitude
and central_latitude
, but then also define center
which has the default value of [central_longitude, central_latitude]
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
.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.
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
.
@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.
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.
@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.
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.
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.
Description of the desired feature
The
Figure
methods take aprojection
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 singleProjection
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 whenstr
is called on them, they produce the GMT-J
code. For example:The plotting method pre-processing would only have to call
str
onprojection
, 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 :)