-
Notifications
You must be signed in to change notification settings - Fork 224
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
Comments
@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 |
@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 The problem with To avoid cluttering the namespace, I'd be fine with bundling them in a submodule like |
I definitely like @MarkWieczorek's names for the projections. Though since we have classes they should be CamelCase. |
Ok noted - basically do |
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)
For any parameters listed as optional on GMT, such as The Enum class >>> 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. >>> 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 |
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 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 |
Just a couple minor comments:
|
Thanks for the feedback @weiji14 and @MarkWieczorek |
I only mentioned |
@sixy6e thank you for doing all of this!
What is the rationale for the I'll leave some more comments in #379. @MarkWieczorek a few comments to your points:
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
That is a great idea! I have having to specify a string for something that should be a number (
Classes should always be CamelCase but module-level constants are UPPER_CASE.
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 |
Hi @leouieda Thanks for your comments. The rationale for the I agree with your comment on the 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 |
@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 :)
The text was updated successfully, but these errors were encountered: