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

In some functions, G is aliased as "fill" but shown "color" in docstrings #1617

Closed
Tracked by #2244
mdtanker opened this issue Nov 8, 2021 · 10 comments
Closed
Tracked by #2244
Labels
documentation Improvements or additions to documentation
Milestone

Comments

@mdtanker
Copy link
Contributor

mdtanker commented Nov 8, 2021

Noticed a mistake here: -G (fill) is listed as "color" under the "Parameters" section, should be "fill".

@weiji14 weiji14 added the documentation Improvements or additions to documentation label Nov 9, 2021
@weiji14
Copy link
Member

weiji14 commented Nov 9, 2021

Right, so the code aliases -G as fill:

G="fill",

But we used {G} in the docstring here:

which means it shows up in the docs as color:

"G": """\
color : str or 1d array

image

So, do we go with using color or fill?

@maxrjones
Copy link
Member

xref GenericMappingTools/gmt#5563 - I think we're leaning towards fill for GMT, but PyGMT already has color in plot and plot3d.

@weiji14
Copy link
Member

weiji14 commented Nov 9, 2021

xref GenericMappingTools/gmt#5563 - I think we're leaning towards fill for GMT, but PyGMT already has color in plot and plot3d.

Yeah, I think it would be nice to use fill too to standardize with GMT. Just that the color="G" alias for plot/psxy has been here for 4 years already since

G='color')
so this would be a pretty significant deprecation. Ok with deprecating this though, because 'color' is a pretty generic term, and 'fill' is more specific to mean just the inside of the symbol/polygon being plotted. What do others think?

@willschlitzer
Copy link
Contributor

I think fill works better, but it should be changed across the PyGMT modules.

@seisman seisman added this to the 0.6.0 milestone Jan 5, 2022
@seisman seisman mentioned this issue Jan 5, 2022
5 tasks
@seisman
Copy link
Member

seisman commented Jan 5, 2022

I also agree that fill is better.

I think it's possible to extend the use_alias decorator to support multiple aliases.
For example, @use_alias(G=["fill", "color"]) means both fill are color are accepted.

@maxrjones
Copy link
Member

I prefer that we don't indefinitely support multiple aliases. The deprecate_parameter decorator is already designed to allow two parameter names, only temporarily with a warning. My preferred path forward would be to update the parameter name of "G" in the COMMON_OPTIONS dict in pygmt/helpers/decorators.py from color to fill and add the deprecate_parameter decorator to plot and plot3d with a long deprecation time-frame (e.g., 4 release cycles). This path forward would not require any changes to #1431.

@weiji14 weiji14 modified the milestones: 0.6.0, 0.7.0 Mar 11, 2022
@seisman seisman changed the title Suggested improvement for api/generated/pygmt.Figure.histogram In some functions, G is aliased as "fill" but shown "color" in docstrings May 6, 2022
@seisman seisman modified the milestones: 0.7.0, 0.8.0 Jun 23, 2022
@seisman
Copy link
Member

seisman commented Sep 25, 2022

I believe the documentation issue has been fixed by #2124.

Now the question is, do we want to deprecate color in plot and plot3d, as suggested in #1617 (comment)

@seisman seisman closed this as completed Sep 25, 2022
@seisman seisman reopened this Sep 25, 2022
@willschlitzer
Copy link
Contributor

I believe the documentation issue has been fixed by #2124.

Now the question is, do we want to deprecate color in plot and plot3d, as suggested in #1617 (comment)

If the documentation says fill, I assume we should deprecate color? I agree with @maxrjones that we shouldn't indefinitely support multiple aliases.

@weiji14
Copy link
Member

weiji14 commented Sep 26, 2022

I believe the documentation issue has been fixed by #2124.
Now the question is, do we want to deprecate color in plot and plot3d, as suggested in #1617 (comment)

If the documentation says fill, I assume we should deprecate color? I agree with @maxrjones that we shouldn't indefinitely support multiple aliases.

The documentation for fig.histogram and fig.ternary uses the fill alias for -G, but fig.plot and fig.plot3d uses color as the alias, so there's inconsistency. I say we deprecate color as Max recommended above, and do so over 4 minor release cycles, since plot/plot3d are widely used.

@seisman
Copy link
Member

seisman commented Oct 30, 2022

I'll work on this issue in a series of PRs:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

5 participants