search

GenericMappingTools / pygmt

A Python interface for the Generic Mapping Tools.
https://www.pygmt.org
BSD 3-Clause "New" or "Revised" License
758 stars 220 forks source link

Consistent (non-common) aliases between modules #1042

Closed maxrjones closed 6 months ago

maxrjones commented 3 years ago

Description of the problem

The -C arguments (parameters) in contour and grdcontour do essentially the same thing. But, PyGMT uses different aliases for these parameters in the two methods (interval and level). I think it would be better to use the same aliases for options that are very similar across the various modules. This is harder to track for non-common options, so perhaps we can also create a tool for identifying similar GMT arguments across modules.

willschlitzer commented 3 years ago

I'm not familiar with the use of the -C in contour or grdcontour and don't want to cause any breaking changes before v0.3.1, but they have very different documentation. In the future I think we should standardize names, but for now, should we copy the interval documentation from grdcontour.py and copy it into levels in contour.py? I notice that interval states it can accept an integer or string, while levels just states that it can accept a string. I'm assuming that levels can accept an integer as well?

maxrjones commented 3 years ago

I'm not familiar with the use of the -C in contour or grdcontour and don't want to cause any breaking changes before v0.3.1, but they have very different documentation. In the future I think we should standardize names, but for now, should we copy the interval documentation from grdcontour.py and copy it into levels in contour.py? I notice that interval states it can accept an integer or string, while levels just states that it can accept a string. I'm assuming that levels can accept an integer as well?

Works for me:

import pygmt
import numpy as np

np.random.seed(42)
region = [0, 10, 0, 12]
x = np.random.uniform(region[0], region[1], 100)
y = np.random.uniform(region[2], region[3], 100)
z = np.sqrt(x)*np.sqrt(y)

fig = pygmt.Figure()
fig.basemap(region=region, projection="X15c", frame=True)
pygmt.makecpt(cmap="viridis", series=[z.min(),z.max()])
fig.plot(x, y, style="c0.2c", cmap=True, color=z)
fig.contour(x, y, z, levels=1,pen="thin")
fig.show()
willschlitzer commented 3 years ago

Sounds good. I'll open a PR to copy the documentation.

seisman commented 6 months ago

I am still not too happy with the aliases for C of Figure.grdcontour (interval) and of Figure.contour (levels):

So, maybe contours would be a better alias for C (see also https://docs.generic-mapping-tools.org/dev/grdcontour.html#c)? Maybe we should complete the PRs #2706 and #3116 regarding the allowed input format, and then discuss this in more detail if these two deprecations are worth it?

I agree that these two wrappers should have the same parameter names, but I'm not sure if we should use contours. For reference, the matplotlib one also uses levels: https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.contour.html

Let's continue the discussions at https://github.com/GenericMappingTools/pygmt/pull/2706#issuecomment-2079279335. It would be better if we can reach an agreement about the consistent parameter name and deprecate the old name in v0.12.0 and fully remove it in v0.16.0 considering that grdcontour/contour are commonly used. Ping @GenericMappingTools/pygmt-maintainers

yvonnefroehlich commented 6 months ago

I am still not too happy with the aliases for C of Figure.grdcontour (interval) and of Figure.contour (levels):

So, maybe contours would be a better alias for C (see also https://docs.generic-mapping-tools.org/dev/grdcontour.html#c)? Maybe we should complete the PRs #2706 and #3116 regarding the allowed input format, and then discuss this in more detail if these two deprecations are worth it?

I agree that these two wrappers should have the same parameter names, but I'm not sure if we should use contours. For reference, the matplotlib one also uses levels: https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.contour.html

Let's continue the discussions at #2706 (comment). It would be better if we can reach an agreement about the consistent parameter name and deprecate the old name in v0.12.0 and fully remove it in v0.16.0 considering that grdcontour/contour are commonly used. Ping @GenericMappingTools/pygmt-maintainers

Yes, I also thought about getting this in v0.12.0. I think the important point is to have the same alias for C of Figure.grdcontour and Figure.contour. levels is already used for Figure.contour, GMT Julia also supports this alias, and matplotlib uses levels. So, I think I am OK with this alias for C if the other @GenericMappingTools/pygmt-maintainers prefer it.

weiji14 commented 6 months ago

Let's continue the discussions at #2706 (comment). It would be better if we can reach an agreement about the consistent parameter name and deprecate the old name in v0.12.0 and fully remove it in v0.16.0 considering that grdcontour/contour are commonly used. Ping @GenericMappingTools/pygmt-maintainers

Yes, I also thought about getting this in v0.12.0. I think the important point is to have the same alias for C of Figure.grdcontour and Figure.contour. levels is already used for Figure.contour, GMT Julia also supports this alias, and matplotlib uses levels. So, I think I am OK with this alias for C if the other @GenericMappingTools/pygmt-maintainers prefer it.

Yep, ok with using levels across both contour and grdcontour.

seisman commented 6 months ago

@yvonnefroehlich Could you please open a PR to deprecate interval to levels in Figure.grdcontour?

yvonnefroehlich commented 6 months ago

@yvonnefroehlich Could you please open a PR to deprecate interval to levels in Figure.grdcontour?

Submitted PR #3209 for this.