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

Consistent (non-common) aliases between modules #1042

Closed
maxrjones opened this issue Mar 12, 2021 · 8 comments · Fixed by #3209
Closed

Consistent (non-common) aliases between modules #1042

maxrjones opened this issue Mar 12, 2021 · 8 comments · Fixed by #3209
Labels
deprecation Deprecating a feature
Milestone

Comments

@maxrjones
Copy link
Member

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.

@maxrjones maxrjones added the enhancement Improving an existing feature label Mar 12, 2021
@willschlitzer
Copy link
Contributor

willschlitzer commented Mar 12, 2021

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
Copy link
Member Author

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
Copy link
Contributor

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

@seisman
Copy link
Member

seisman commented Apr 28, 2024

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

@yvonnefroehlich
Copy link
Member

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
Copy link
Member

weiji14 commented Apr 28, 2024

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
Copy link
Member

seisman commented Apr 29, 2024

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

@seisman seisman added deprecation Deprecating a feature and removed help wanted Helping hands are appreciated enhancement Improving an existing feature labels Apr 29, 2024
@seisman seisman added this to the 0.12.0 milestone Apr 29, 2024
@yvonnefroehlich
Copy link
Member

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

Submitted PR #3209 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Deprecating a feature
Projects
None yet
5 participants